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.
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... |
rwjblue
force-pushed
the
bugfix/wait-for-transitions
branch
from
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
rwjblue
force-pushed
the
bugfix/wait-for-transitions
branch
from
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...
rwjblue
force-pushed
the
bugfix/wait-for-transitions
branch
from
4a4f795
to
d399dda
Compare
rwjblue
changed the title
|
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.