Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CLEANUP beta] Remove deprecate-router-events support code #19749

Merged
merged 3 commits into from
Nov 5, 2021

Conversation

xg-wang
Copy link
Contributor

@xg-wang xg-wang commented Sep 9, 2021

Deprecation https://deprecations.emberjs.com/v3.x/#toc_deprecate-router-events

This PR removes

  • OverridingwillTransition and didTransition hooks on EmberRouter
  • willTransition and didTransition events on Route

Based on https://github.com/emberjs/rfcs/blob/af64915b5ecde010fce09309a47ee6d2447588d0/text/0095-router-service.md#transition-object I think willTransition and didTransition on Route are not deprecated.
They are triggered from https://github.com/tildeio/router.js/blob/4f5d411e9ba44efea003bc3159484b4060ebfc2a/lib/router/router.ts#L811

@snewcomer
Copy link
Contributor

note ref - #19709

Either one is fine by me!

@xg-wang
Copy link
Contributor Author

xg-wang commented Sep 11, 2021

It looks like #19709 removed the deprecation warnings but still triggers those events if app has the route event. But it also appears I missed removing used test code part, I picked those to this PR.

@xg-wang
Copy link
Contributor Author

xg-wang commented Sep 11, 2021

@rwjblue I checked the router service RFC and I think actions on Route are not deprecated

assert(
'You attempted to override the "didTransition" method which has been deprecated. Please inject the router service and listen to the "routeDidChange" event.',
router.didTransition === defaultDidTransition
);
router.didTransition(infos);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we can take this a step further and make defaultDidTransition && defaultWillTransition the defacto method that is called when didTransition/willTransition is called on the PrivateRouter. I'm wondering if we can take everything in, say defaultDidTransition and just throw it in here and then get rid of the assert.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xg-wang what do you think?

Copy link
Contributor Author

@xg-wang xg-wang Oct 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@snewcomer @nlfurniss sorry for the late reply.

The problem of putting everything from defaultDidTransition to here is that there won't be assertion thrown when people who used to override the didTransition. Before this PR there are warnings.

My thought is that we should internally migrate from router_js's didTransition to routeDidChange then major bump router_js to remove didTransition/willTransition. Meanwhile keep the defaultDidTransition.

@xg-wang xg-wang mentioned this pull request Oct 21, 2021
58 tasks
@xg-wang
Copy link
Contributor Author

xg-wang commented Oct 23, 2021

Rebased to resolve a conflict

@rwjblue rwjblue changed the title [CLEANUP beta] deprecate-router-events [CLEANUP beta] Remove deprecate-router-events support code Nov 5, 2021
@rwjblue rwjblue merged commit 4f120c8 into emberjs:master Nov 5, 2021
@xg-wang xg-wang deleted the xg-wang/router-events branch November 5, 2021 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants