fix(router): restore 'history.state' on popstate even if navigationId missing

[nebkat]

If history.pushState() or history.replaceState() were called manually without including the navigationId field the state was being incorrectly discarded. That logic was meant for maintaining the original behavior of NavigationStart.restoredState, not for restoring the state to history.state.

Improves on #28176, fully fixes #28108, see also #28954.

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/angular/angular/blob/main/CONTRIBUTING.md#commit
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

• Bugfix
• Feature
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no api changes)
• Build related changes
• CI related changes
• Documentation content changes
• angular.io application / infrastructure changes
• Other... Please describe:

What is the current behavior?

history.replaceState({ foo: 'bar' });

location.back(); // Or browser button
location.forward();  // Or browser button

history.state; // contains { navigationId: ... }

Issue Number: #28954

What is the new behavior?

history.replaceState({ foo: 'bar' });

location.back(); // Or browser button
location.forward();  // Or browser button

history.state; // contains { navigationId: ..., foo: 'bar' }

Does this PR introduce a breaking change?

• Yes
• No

Only if app was relying on the broken behavior already.

Other information

See also #28954 (comment)

[atscott]

Using location.replaceState outside of the Router would break scroll restoration and the page id used to restore history correctly if a traversal navigation is rejected by guards. That said, I don't see any reason for the Router to blow away the state like it's doing now so this looks like a good change to me. Thanks!

[atscott]

"restoredState is used in the NavigationStart event. The stored navigationId is used by the RouterScroller to retrieve the scroll position for the page."

[nebkat]

I've kept the original comment just to clarify that the behavior must be maintained for apps that rely on the { state } | null. RouterScroller uses navigationId but doesn't fail if it is missing.

[atscott]

I don't follow what you mean here, nor do I follow what the comment means to accomplish. The comment does not make the code more clear to me and actually makes it more confusing.

RouterScroller uses navigationId but doesn't fail if it is missing.

How is that possible? restoredState and navigationId is required to make scroll restoration work.

[nebkat]

The original motivation for this fix was to preserve the contents of history.state if it is missing navigationId, which you agree with:

I don't see any reason for the Router to blow away the state like it's doing now

The only reason we do still blow away the state for NavigationStart.restoredState is to preserve the logic from pre-#27198.

Pre #27198: restoredState only contained navigationId, so it made sense to store null if it wasn't present.
Post #27198 restoreState contains the entire state, no longer makes sense to discard it (applying same logic as above).

Despite that, the logic must still be preserved in case an app has the following code:

if (e instanceof NavigationStart) {
    if (e.restoredState === null) doSomething();
}

The comment just makes that explicitly clear, since internal dependencies (i.e. RouterScroller) may change in future causing even more confusion.

How is that possible? restoredState and navigationId is required to make scroll restoration work.

Sorry what I meant by this was that RouterScroller does not throw an error if restoredState is missing navigationId. The scroll restoration of course doesn't work, but it silently reads undefined and ignores it.

[atscott]

Ok, so I do agree partially with the goal of the comment. That said, right now it kind of reads like “this code was added in PR ‘x’. Changing it might/would break things.” Which is always true. Let’s instead make this actionable:

“TODO: restoredState should always include state if it exists, regardless of navigationId. This requires a breaking change to update the type on NavigationStart’s restoredState type, which requires navigationId to always present. The Router used to only restore history state if a navigationId were present.”

[nebkat]

Sounds good, apologies for the confusion!

[cyraid]

Any update on this?

[atscott]

Setting review state to request changes to address comment

[nebkat]

Would it be useful if I sent in more PRs for some of the router TODOs? I would love to help out further but I wouldn't want to waste your time reviewing minor improvements if it will require lots of extra discussion. I can think of some nice long term solutions to the state handling but they would involve deprecation - might be better left to you?

[atscott]

Would it be useful if I sent in more PRs for some of the router TODOs? I would love to help out further but I wouldn't want to waste your time reviewing minor improvements if it will require lots of extra discussion. I can think of some nice long term solutions to the state handling but they would involve deprecation - might be better left to you?

I'm absolutely open to fielding more community PRs for the Router. I will say that many changes are pretty hard-fought, especially when they have to do with the timing of state management so just be prepared for some of them to be breaking in a way that might make the change very difficult to land. So just be prepared for things to be slow, not move forward, or even get reverted if failures were detected later on.
As for the state handling, if there's a change that leads us towards a better future but requires a deprecation, that's a definitely conversation worth having. Maybe you could start the conversations by opening an issue and we can discuss the changes ahead of time so you don't waste too much time on the implementation if you aren't sure we'd want to make the change.

[ChanduMdp]

Caretaker Note: Please ignore the google-internal-tests status. All targets passed after rerunning

[cyraid]

Would it be useful if I sent in more PRs for some of the router TODOs? I would love to help out further but I wouldn't want to waste your time reviewing minor improvements if it will require lots of extra discussion. I can think of some nice long term solutions to the state handling but they would involve deprecation - might be better left to you?

Thank you @nebkat for all your work on this. :)

[cyraid]

Setting review state to request changes to address comment

And thank you @atscott for following up so quickly.

[dylhunn]

This PR was merged into the repository by commit 1976e37.

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}