Fix URL Update Method for QP-replacement during Transition

[alexlafroscia]

As part of investigating what parts of the failing tests in emberjs/ember.js#19092 might be part of router.js -- as a means of getting to a solution for emberjs/ember.js#18416 -- I wrote up these tests to try to isolate the behavior that should be happening here.

Essentially, it seems to me that if you perform a new "replacement" transition during an active "update" transition which only changes the query params, the browser should only push one item into the History stack; a URL update to the URL with the query params.

However, that's not what actually happens; in reality, a replaceURL happens from the "replacement" transition and then the updateURL happens. This gets the browser's Back button all kinds of confused.

[alexlafroscia]

Part of what would be really helpful in resolving this would be to clarify the expected sequence of events.

I would assume that when a new QP-only transition occurs during another transition, the active transition is aborted in favor of the new transition, like normal. However, that's specifically not what happens. Instead, the new query params are assigned to the active transition and both are allowed to complete.

Does it make sense that that's the behavior?

I was able to get the code to do what I would have expected to happen, but other tests started to fail when I made that change. This really has me wondering what the correct behavior is supposed to be in a case like this.

For example, this test ensures that when you transition to route foo, which replaces itself with route bar, that the ultimate URL update method used is replaceURL

https://github.com/alexlafroscia/router.js/blob/fc2177d52c15ae77c6f728108b98be0f08285b0e/tests/router_test.ts#L5707-L5750

I would think that it actually should be call updateURL with the correct value for bar in this case instead. With the fix that I have locally, this test fails but my new ones pass... which might be correct? 🤷‍♂️

[alexlafroscia]

I decided to go ahead and push the code I have written toward fixing this error so that it can be looked at.

The change here is essentially that

1. If a QP-only transition interrupts another transition, we actually redirect to the new transition and abort the one in-progress. Previously, both transitions were executed, which is where the extra replaceURL call comes from.
2. A transition now keeps track of whether the one that it replaces intended to update the URL
3. If a "replacement" transition gets to the point where it attempts to change the URL, it will perform an updateURL if it was created by interrupting a transition that intended to update the URL. This ensures that a URL update does in fact take place.

If it seems like the existing tests are ensuring the incorrect behavior, I'm happy to go and update them to match the new behavior. For now, I want to leave them alone as a discussion point.

[rwjblue]

If it seems like the existing tests are ensuring the incorrect behavior, I'm happy to go and update them to match the new behavior. For now, I want to leave them alone as a discussion point.

Can you push an additional commit updating those tests just so its easier to review and try to grok the impact to apps?

[buschtoens]

I'm attempting to fix a semi-related bug (emberjs/ember.js#18577) in #307.

- let newTransition = new InternalTransition(this, undefined, undefined);
+ let newTransition = new InternalTransition(this, undefined, newState);

[sly7-7]

@rwjblue I just tried to dig into this. I've rebased against master (more exactly against https://github.com/sly7-7/router.js/commits/fix-ember-19266), and I think those remaining failing tests

are the same as those @alexlafroscia were talking about. So what's the status of this ? I don't know what is expected, neither exactly the difference between replaceUrl/updateUrl (I guess either the browser will either replace the current history url/ push a new state ?).

[sly7-7]

@alexlafroscia Could you look at this "interesting" example please ? emberjs/ember.js#11563 (comment)

I'm navigating through router issues, and for now this PR seems to fix some of them, but this #11563 is still failing. (or maybe it allows an other underlying issue to surface)

[rwjblue]

Having just reviewed the changes here again, I think the code here is good overall (and the right direction). We need to dig into those test failures to decide if they are legit or just mistaken assertions.

[sly7-7]

@rwjblue Thank you for the review. I were investigating emberjs/ember.js#11563 and realize that a test I just wrote in ember.js code looks the same as in emberjs/ember.js#19092, causes the same error Cannot read property name of undefined. So potentially, also the same root cause as in emberjs/ember.js#19417.

I can eventually push this forward if @alexlafroscia doesn't have the time. But as I said, I don't know what's the expected behavior, and also don't have a good knowledge about the differences between replaceUrl/updateUrl and what that means for the browser and the end user (Yes, I don't have the basis on web dev). I guess this also depends on the locationType used in the app.
Anyway, you can ping me on discord we you have time to discuss about it

[alexlafroscia]

Hey everyone, sorry I've been MIA as this conversation picks up again. It's been a while since I was knee-deep in this problem and I've sort of lost context on what I was doing.

@sly7-7 if you're working through this now, feel free to pick it up where I put it down. If you want to take some time to chat through the decisions that need to be made around the tests or pair on something, I'm happy to set up some time for that this week!

[sly7-7]

@alexlafroscia Thank you for your answer. I'm currently navigating trough a lot (at least for me it's a lot) of issues in the router. I'm on other ones right now + trying to understand how routing works. But definitely, I think this PR is relevant, and no doubt I will come back later on it.

[wagenet]

@sly7-7 status?