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

test: add failing test around replaceWith and refreshModel #19092

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alexlafroscia
Copy link

@alexlafroscia alexlafroscia commented Aug 12, 2020

I've been trying to see if I can bet to the bottom of what is going on in #18416. I figured I would start with a failing test and work backward from there, but I couldn't get very far before hitting a different failure altogether.

I don't intend to land the test in this partial state, but hope that this can be a starting place for actually fixing this bug.

I want want the test to verify is that, when calling replaceWith in afterModel, the router does not unintentionally push a new state into the history stack, which is what actually happens. However, before I can even get to that kind of assertion in the test, I run into this error:

Screen Shot 2020-08-12 at 5 11 23 PM

(Screenshot from an ember twiddle I created to make sure that this error was actually happening in a "real" Ember app and not just the test suite. The test suite will fail with the same error.)

What appears to be happening is that the Transition object is created without any routeInfos

https://github.com/tildeio/router.js/blob/d885da22340da98dcadb45427766efade54ce832/lib/router/router.ts#L123-L130

but then those routeInfos are passed around to a number of functions that expect them to exist as a side-effect of calling finalizeQueryParamChange.

I am going to assume that this is a fix that will need to at least start in router.js and then continue through to here, but figured I would at least post this PR as some small step toward getting to an actual fix.

@kategengler
Copy link
Member

Is this still relevant?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants