ember-cli-document-title icon indicating copy to clipboard operation
ember-cli-document-title copied to clipboard

Prefer to obtain router directly from container, over the route

Open mike-north opened this issue 5 years ago • 19 comments

In order to avoid future breakage in Ember 3.5, possible collision w/ user-defined properties, and deprecation warnings in Ember 3.1, this small change attempts to obtain the router from the container instead of from the route.

Related deprecation: https://www.emberjs.com/deprecations/v3.x/#toc_ember-routing-route-router

mike-north avatar Jul 09 '18 02:07 mike-north

@kimroen BOOM! Lets get this merged!

Duder-onomy avatar Jul 25 '18 17:07 Duder-onomy

"ember-cli": "^3.4.0-beta.1"

Uncaught TypeError: self.router.setTitle is not a function at document-title.js:75 at tryCatcher (rsvp.js:200) at invokeCallback (rsvp.js:372) at publish (rsvp.js:358) at rsvp.js:14 at invoke (backburner.js:205) at Queue.flush (backburner.js:125) at DeferredActionQueues.flush (backburner.js:278) at Backburner.end (backburner.js:410) at Backburner._run (backburner.js:760)

Argun avatar Jul 27 '18 03:07 Argun

@Argun the ember version is more relevant than ember-cli verision, and any optional features and feature flags you may have turned on

mike-north avatar Jul 27 '18 16:07 mike-north

@mike-north which optional flags need turn on?

Argun avatar Aug 01 '18 04:08 Argun

@Argun I took the time to update all dependencies of this library to current (#76) based on your report that this doesn't work in Ember 3.4.0-beta.1. Now this addon's test suite runs for a bunch of different dependency scenarios ranging from Ember 1.13.x through 3.5.0-canary.

If you still believe this change to be problematic, please create an ember app that demonstrates the issue, publish it on github and respond with a link to it so I can take a closer look. As it stands, I have nowhere near enough information to help you out.

mike-north avatar Aug 01 '18 07:08 mike-north

Hey @mike-north @kimroen ! is there any blocker to merge this PR?

dmuneras avatar Sep 13 '18 20:09 dmuneras

@dmuneras you can always point to my branch in your package.json.

{
"ember-cli-document-title": "https://github.com/mike-north/ember-cli-document-title#route-router"
}

mike-north avatar Sep 13 '18 20:09 mike-north

@mike-north This looks great. Tiny point, should you also update the package version?

BitBrit avatar Sep 17 '18 17:09 BitBrit

Tiny point, should you also update the package version?

@BitBrit When contributing to libraries maintained by others, I don't want to presume what kind of version change will occur. There could be several releases before this PR gets merged in, and a version that might make sense today would not make sense at the time the code is merged.

mike-north avatar Sep 17 '18 17:09 mike-north

@mike-north Yep, that seems fair. Although.....there doesn't seem to have been much action on that front for a while. It would be good to get these changes merged.

BitBrit avatar Sep 17 '18 17:09 BitBrit

@kimroen can this get some attention? Would like to get rid of the deprecation warning.

rmachielse avatar Sep 26 '18 10:09 rmachielse

Also would like to see this merged...

lepolt avatar Oct 11 '18 11:10 lepolt

Hi 😊

Is there anything preventing this PR from being merged? If so is there anything we can do to help?

Thanks! ❤️

simonc avatar Oct 26 '18 20:10 simonc

I believe this PR should be superseded by #80.

scalvert avatar Nov 15 '18 16:11 scalvert

@scalvert The challenge is getting any of them merged.

btecu avatar Nov 15 '18 16:11 btecu

I've pulled most of these commits (and equivalent others) into https://github.com/mike-north/ember-cli-document-title-northm

and released ember-cli-document-title-northm v1. In the event that this library once again is maintained, I'm happy to deprecate/retire my package.

mike-north avatar Nov 24 '18 00:11 mike-north

Spoke to @kimroen via Twitter. He's on vacation right now, but will look into all PRs once back.

scalvert avatar Nov 28 '18 02:11 scalvert

Thanks for letting us know! Side note that according to Twitter as of when I write this, he uses “he/his” pronouns. 💞

backspace avatar Nov 28 '18 02:11 backspace

Corrected! Thanks, @backspace!

scalvert avatar Nov 28 '18 03:11 scalvert