Fix router event deprecations, update to Ember 3.7 #183
Conversation
RobbieTheWagner
changed the title
| @@ -1,6 +1,6 @@ | |||
| The MIT License (MIT) | |||
|
|
|||
| Copyright (c) 2016 | |||
| Copyright (c) 2019 | |||
addon/index.js
Outdated
| scrollPosition = { x: hashElement.offsetLeft, y: hashElement.offsetTop }; | ||
| } else { | ||
| scrollPosition = get(this, 'service.position'); | ||
| } | ||
|
|
||
| const preserveScrollPosition = transitions.some((transition) => get(transition, 'handler.controller.preserveScrollPosition')); | ||
| const preserveScrollPosition = get(transition, 'handler.controller.preserveScrollPosition'); |
|
@snewcomer I am not sure when this new API was introduced. It is possible it has always existed. |
|
Thanks @rwwagner90, this is one of the deps (transitive through ember-cli-addon-docs) that has been breaking ember-animated on ember 3.6+. I think this deprecation is fallout from the work that implemented the router service RFC. IIRC, these deprecated methods receive arguments that leak a bunch of internals and were never properly designed or documented. The new events receive arguments that don't require you to crawl through handlerInfos, etc, to discover what's going on. |
|
@ef4 do you know what versions of Ember the new methods should work with? I think the main concern here is whether we need to keep the old way too and conditionally call one or the other based on Ember version or if the new way works pretty far back in Ember as well? |
|
Perhaps add 2.16 and then 2.12 to the travis config and see which one/if any fails? |
|
@snewcomer we can definitely test back to those versions, but I am concerned perhaps the tests are not truly testing this. It would be great to know the Ember version support for the router events. @ef4 @rwjblue and insights on this? |
|
Ember added these new router events in 3.6. |
| ``` | ||
|
|
||
|
|
||
| ## Running Tests |
There was a problem hiding this comment.
This seems like a thing for CONTRIBUTING.md (along with the greenkeeper mention)
addon/index.js
Outdated
|
|
||
| destroy() { | ||
| reset(); | ||
| this.on('routeWillChange', () => { |
There was a problem hiding this comment.
This change definitely is a breaking change (maybe thats fine?) as it only supports 3.6+
If you need to stay compatible, you would bring in ember-compatiblity-helpers and wrap these things in an if:
import { gte } from 'ember-compatibility-helpers';
// ...snip...
if (gte('3.6.0-beta.1')) {
// code for new APIs
} else {
// code for older deprecated APIs
}During a production build, the unused condition will be completely stripped (meaning there is no additional bloat to applications by having these conditionals).
There was a problem hiding this comment.
Thanks @rwjblue I will do that. It's odd that all our tests are passing on older Ember. They must not be truly testing things.
|
I don't know, you'd have to evaluate the tests themselves to see if they are testing reality or just unit testing with stubs (or some combo of each). |
|
@snewcomer @rwjblue I think I got everything wrapped in compatibility helpers. I am adding |
addon/index.js
Outdated
| }); | ||
| } else { | ||
| this.willTransition = (...args) => { | ||
| this._super(...args); |
There was a problem hiding this comment.
I don't think this._super will work in this context (because this arrow function won't be super wrapped which happens during .extend).
There was a problem hiding this comment.
I think generally speaking we want to restructure to something like:
let RouterScrollMixin = Mixin.create({
// shared code, and new `init` (with `router.on('routeWillChange'` / `router.on('routeDidChange'` behind the gte guard)
});
if (!gte('3.6.0-beta.1')) {
RouterScrollMixin = Mixin.create(RouterScrollMixin, {
willTransition() {},
didTransition() {},
});
}
export default RouterScrollMixin;There was a problem hiding this comment.
Makes sense, will update
|
@rwjblue updated per your suggestions 👍 |
|
I am using it in a 3.7 app, and the changes work nicely. @snewcomer do you have an app on something before 3.6 you use this addon in and could test with? |
|
Sounds good @snewcomer! Are you able to merge or do we need to wait for The DSC guys? |
|
We don't do a bump to release after merging? Typically it's a separate commit and tag via |
|
@rwwagner90 sure. That works too. |
|
Sounds good. Please merge at your convenience then 😄 |
|
Can someone with npm permissions please bump the version and release? |
|
On my quest to fix ember-animated I found that this is the last addon preventing the update. Could we get a new release including these changes? |
|
@snewcomer @yowainwright if you guys would be interested in giving me my permissions back, I would be happy to handle the release. |
|
+1 for a release with this! 😀 |
|
Still happy to release this myself, if I can get permissions back please. |
|
I don’t have permissions to publish. Just on github. Thx a ton for this work. If you don’t have publish permissions, I bet Jeff will get to it soon! |
|
@snewcomer @yowainwright Can you guys please release it? |
|
There is a PR up, but @snewcomer and myself do not have permissions to npm #187 |
didTransitionandwillTransitionhave been deprecated. It is now recommended to use event handlers instead. See https://deprecations-app-prod.herokuapp.com/deprecations/v3.x/#toc_deprecate-router-eventsIt also seemed like Travis was not running ember-try, so the tests were only running on the most recent Ember version. In an attempt to remedy this, I updated everything with ember-cli-update.
Closes #181