-
-
Notifications
You must be signed in to change notification settings - Fork 255
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
Wait for pending route transitions as part of settledness check #482
Wait for pending route transitions as part of settledness check #482
Conversation
Oh, no! I had started to work on fixing this bug over the holiday weekend (US Thanksgiving). You can see my WIP over in master...rwjblue:settled-router-transitions if you are curious. Shall we compare notes? |
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.
Thank you again for picking this up! I've left some inline notes about specific areas for changes, but I'd still like to coordinate on the best way to drive this forward.
I'm happy to port my more "realistic" tests (in place of the mocking tests added here) then look into porting the other features (noop unless visit
is called, using only public API's for "Active transition" when possible, etc) over into this PR if you'd like...
Then we can all three tag team issues that crop up, what do you think?
OK, cool, I'll get some of my branches changes landed into this PR in the next hour or so... |
4d2b3e5
to
6b84ee9
Compare
These tests are designed to mimick real world application scenarios, and confirm that the transitions triggered within each are properly waited for before a `settled()` promise resolves.
The code added in an earlier commit was working around the underlying issue. Essentially, using `getContext` in `hasPendingTransition` meant that we were always using whatever the "last" context was and since every test does not (and **should not**) call `setContext` we were accidentally sharing an older tests context. This change ensures we `setContext` up before these `setupApplicationContext` tests (which is important), but we _also_ should add more changes to the testing harness in @ember/test-helpers overall to ensure that each test internally cleans up the context (look out for future PR's/commits for that)...
…solve some PR comments
17c604e
to
7aa6a57
Compare
Rebased with master to fixup conflicts. |
This allows `setupTest(hooks)` tests that use `this.owner.setupRouter()` to also take advantage of the settledness tracking.
The way we are hooking into the application instance registry ultimately means that we leak "factories". Once the first test resolves a factory for a given name, **all other tests** will use that factory (even if `setResolverRegistry` were called again with a different value!1!1!1!1). This works around the immediate issue by always using the "current" assert object instead of the closure scoped one because all of the tests will use the same factory, and using `assert` from the first test will cause many issues for subsequent tests. In the long run, we need to fix this in a different way. Most likely that means changing the way setResolverRegistry works to do something like: * hooking into the shared application instance * calling unregister and reregister for each key in the pojo provided Alternatively, we could recreate the registry instance itself (therefore flushing any caches). I think either would work, but am not certain how hard to implement...
4a4f795
to
d399dda
Compare
Thank you again @dexturr and @AlexTraher! Your work (and debugging) was very very helpful in getting this over the line... |
This aims to resolve an issue where
settled()
isn't waiting for router transitions. See here: #411.This is a combined effort from myself and @dexturr.