kimroen / ember-cli-document-title

Adding document title behaviour to your ember app
MIT License
216 stars 61 forks source link

Prefer to obtain router directly from container, over the route #75

Open mike-north opened 6 years ago

mike-north commented 6 years ago

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

Duder-onomy commented 6 years ago

@kimroen BOOM! Lets get this merged!

Argun commented 6 years ago

"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)

mike-north commented 6 years ago

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

Argun commented 6 years ago

@mike-north which optional flags need turn on?

mike-north commented 6 years ago

@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.

dmuneras commented 6 years ago

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

mike-north commented 6 years ago

@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"
}
BitBrit commented 6 years ago

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

mike-north commented 6 years ago

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.

BitBrit commented 6 years ago

@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.

rmachielse commented 6 years ago

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

lepolt commented 5 years ago

Also would like to see this merged...

simonc commented 5 years ago

Hi 😊

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

Thanks! ❤️

scalvert commented 5 years ago

I believe this PR should be superseded by #80.

btecu commented 5 years ago

@scalvert The challenge is getting any of them merged.

mike-north commented 5 years ago

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.

scalvert commented 5 years ago

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

backspace commented 5 years ago

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

scalvert commented 5 years ago

Corrected! Thanks, @backspace!