-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[BUGFIX release] Avoid instantiation errors when app/router.js
injects the router service.
#19405
Conversation
295b920
to
7344cd3
Compare
packages/ember/tests/routing/router_service_test/non_application_test_test.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we only need to forward the events when someone calls routerService.on('routeWillChange')
, why don't we implement on
and setup the wiring there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go the route of #19454, and look up the router service during init. That way we can avoid relying on { instantiate: false }
. It does add some small amount of overhead (automatic creation of service:router
), but in practice I don't think that will matter much and the resulting simplicity of the implementation will be easier to maintain.
What do you think @Ravenstine?
@rwjblue That makes sense. Doing |
kk, awesome thanks for taking a look @Ravenstine! |
6dc35c8
to
19ddb89
Compare
19ddb89
to
cf4d7d1
Compare
packages/ember/tests/routing/router_service_test/currenturl_lifecycle_test.js
Outdated
Show resolved
Hide resolved
packages/ember/tests/routing/router_service_test/currenturl_lifecycle_test.js
Outdated
Show resolved
Hide resolved
Thanks a lot to both of you @snewcomer @Ravenstine. This looks good to me. I just don't know what should be the choice for the setupRouter() call. I have my opinion, but I'm so new to the router area that I probably miss something. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, this is great work. Thank you @Ravenstine @snewcomer @sly7-7!
app/router.js
injects the router service.
Related to my previous PR: #19385
And issue: #17791
@rwjblue Since we talked about this with @snewcomer recently, maybe you can weigh in on this approach?
The intention is to prevent an infinite recursion when
service:router
is invoked beforerouter:main
has a chance to be initialized and cached. This PR does it by makingservice:router
more "passive" so that the responsibility of forwardingrouteWillChange
androuteDidChange
events is handled byrouter:main
, as opposed to looking up(and possibly instantiating)router:main
and listening to events on it fromservice:router
. The active behavior ofservice:router
instantiatingrouter:main
seems unexpected, and doesn't appear to be required in any tests.Specifically, the
service:router
has been changed to only userouter:main
if it's already available androuter:main
now forwards those two route events toservice:router
if that service has been instantiated. That way there's no possibility of getting caught in a loop ifservice:router
happens to be used during initialization ofrouter:main
, which can happen unexpectedly when using a service from an add-on that referencesservice:router
.Definitely open to any kind of feedback.