Skip to content

Allow setting history.state when navigating#27198

Closed
jasonaden wants to merge 5 commits intoangular:masterfrom
jasonaden:FW-613_add_data_to_events
Closed

Allow setting history.state when navigating#27198
jasonaden wants to merge 5 commits intoangular:masterfrom
jasonaden:FW-613_add_data_to_events

Conversation

@jasonaden
Copy link
Contributor

@jasonaden jasonaden commented Nov 20, 2018

This feature adds a few capabilities. First, when a popstate event fires the value of history.state will be read and passed into NavigationStart. In the past, only the navigationId would be passed here.

Additionally, NavigationExtras has a new public API called state which is any object that will be stored as a value in history.state on navigation. For example, the object {foo: 'bar'} will be written to history.state here:

router.navigateByUrl('/simple', {state: {foo: 'bar'}});

Additionally, the NavigationExtras can now be read during navigation. We're exposing a new type Navigation that provides information such as the navigation ID, target URL, NavigationExtras passed to the navigateByUrl method, and a string value indicating what triggered the navigation (popstate, imperative, etc).

In order to read this Navigation object, use the router.getCurrentNavigation() function. If there's no navigation currently executing, the value will be null. Continuing from the above example:

navigation = router.getCurrentNavigation();

expect(navigation.trigger).toBe('imperative');
expect(navigation.extras.state).toEqual({foo: 'bar'});`

These features are related to a series of issues regarding adding arbitrary data to navigations (#5217 #8515 #9804 #24617). Additionally, this PR closes #10248 for supporting setting history.state.

closes #25658

@jasonaden jasonaden changed the title Fw 613 add data to events Allow setting history.state when navigating Nov 20, 2018
@mary-poppins
Copy link

You can preview 3dccc75 at https://pr27198-3dccc75.ngbuilds.io/.

@jasonaden jasonaden force-pushed the FW-613_add_data_to_events branch 3 times, most recently from 8859de5 to e78a000 Compare November 29, 2018 18:33
@mary-poppins
Copy link

You can preview e78a000 at https://pr27198-e78a000.ngbuilds.io/.

@jasonaden jasonaden force-pushed the FW-613_add_data_to_events branch from 0fd82ac to 774916e Compare November 29, 2018 19:24
@mary-poppins
Copy link

You can preview 774916e at https://pr27198-774916e.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 0fd82ac at https://pr27198-0fd82ac.ngbuilds.io/.

@jasonaden jasonaden force-pushed the FW-613_add_data_to_events branch from 774916e to 45ee235 Compare November 29, 2018 19:51
@mary-poppins
Copy link

You can preview 45ee235 at https://pr27198-45ee235.ngbuilds.io/.

@jasonaden
Copy link
Contributor Author

Presubmit

@alxhub
Copy link
Member

alxhub commented Nov 29, 2018

LGTM

The only thing I would add is that the commits here could benefit from additional info (e.g. a description of what the old behavior was and the motivation for changing it). One useful thing to add in each commit message in a multi-commit PR like this is a line or two about how the code in the commit is tested.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the purpose of exposing this? if it's just to provide access to the previous url, then why not expose that directly as previousUrl?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because then we have to have 3 copies. The previousInitialUrl, previousExtractedUrl and previousFinalUrl. Also, we may add more data to this over time and exposing it this way means we don't need to bloat the object.

@jasonaden jasonaden force-pushed the FW-613_add_data_to_events branch 2 times, most recently from bf4ca6b to 8dd3a72 Compare November 29, 2018 22:50
@mary-poppins
Copy link

You can preview bf4ca6b at https://pr27198-bf4ca6b.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 8dd3a72 at https://pr27198-8dd3a72.ngbuilds.io/.

@jasonaden jasonaden force-pushed the FW-613_add_data_to_events branch from 8dd3a72 to 5ac9f5a Compare November 29, 2018 22:58
@mary-poppins
Copy link

You can preview 5ac9f5a at https://pr27198-5ac9f5a.ngbuilds.io/.

This value will get written to the `history.state` entry.

FW-613 (related)
This value will get written to the `history.state` entry.

FW-613 (related)
Related to angular#24617
@Airblader
Copy link
Contributor

Airblader commented Dec 16, 2018

@jasonaden This feature looks really cool, thanks. However, as a user I am slightly confused about the lifespan of a "current navigation". If I subscribe to the queryParamMap, is it safe to rely on the current navigation's state as long as I stay synchronous (i.e., no switchMap's to some async observable)? I can't seem to find any documentation regarding the lifecycle of a single navigation.

@sky10p
Copy link

sky10p commented Dec 28, 2018

@jasonaden It does not seem to work, when I use getCurrentNavigation it always returns null.

I leave the test that I have done:

https://stackblitz.com/edit/angular-wgc6ou

Thanks!

@fjauer
Copy link

fjauer commented Jan 10, 2019

@sky10p i got your example working:

just moved the coding from ngOnInit to the constructor

https://stackblitz.com/edit/angular-bupuzn

@sky10p
Copy link

sky10p commented Jan 10, 2019

@fjauer And why does that work? Does the constructor not run before?

Thanks for your time!

@alecvolo
Copy link
Contributor

I think there is a small bug - the history.state object is getting lost when navigate using back/forward buttons.
The fix should be pretty simple- set restored state into the NavigationExtras parameter in the call of scheduleNavigation in Router.setUpLocationChangeListener() method :

        // Navigations coming from Angular router have a navigationId state property. When this
        // exists, restore the state.
        const state = change.state && change.state.navigationId ? change.state : null;
     setTimeout(
            () => { this.scheduleNavigation(rawUrlTree, source, state, {replaceUrl: true, state }); }, 0);

The workaround is to catch NavigationStart event and fire navigation from there:

    this._router.events.pipe(
        filter(t =>t instanceof NavigationStart && (<NavigationStart>t).navigationTrigger == "popstate" ))
      .subscribe((x: NavigationStart) => {
        this._router.navigate([x.url], { state: x.restoredState });
      });

@Airblader
Copy link
Contributor

@alecvolo Please open a (new) issue instead of commenting on closed ones.

@f-aubert
Copy link

Maybe a stupid question? But was this nice feat merged in any official Angular Release? 7.2? Or does it wait for Angular 8?

@Airblader
Copy link
Contributor

@f-aubert It has been released in Angular 7.2.0. Refer to the CHANGELOG file in the root directory of the repository to find changes.

@f-aubert
Copy link

Thanks @Airblader for pointing me to the ChangeLog. I did miss it. I would have expected this Issue to be somehow tagged with 7.2.0, and the Routing Documentation to reflect this feature. But many thanks to all that contributed, it was greatly awaited and needed in my apps.

@nhays89
Copy link

nhays89 commented Mar 21, 2019

Probably should create a new bug, but just wanted to see if this should work when receiving an error response in an interceptor and trying to pass the error message to an error component via the router's state object? I am getting a null object as @sky10p described from the call to getCurrentNavigation() and the code is in the constructor as @fjauer commented.

@jasonaden
Copy link
Contributor Author

Thanks for the feedback on this issue everyone. Good that it's useful for many, sorry about any problems with documentation or problems using the feature.

getCurrentNavigation() will return the currently executing navigation. It will return null if there is no currently executing navigation (in other words, the application is in a stable state).

Internally, there is a reference to lastSuccessfulNavigation, but this isn't exposed publicly. We tend to err on the side of not publicly exposing APIs unless valid use cases are presented. It sounds a bit like what some of you are looking for is an API to get either the currently executing navigation or the previous one.

If someone wants to create a feature request for this with use cases, that would help clarify.

Thanks!

@cogidoo
Copy link

cogidoo commented Mar 25, 2019

Why is this only implemented for the RouterLinkWithHref-directive. So currently it's not possible to use the state-feature with a routerlink-directive at - let's say - a button.

@colmben
Copy link

colmben commented Mar 28, 2019

By saying this works only on the currently executing navigation, does that mean it shouldn't be used outside of the navigation lifecycle hooks? It doesn't work by the time ngInit is called in a component for example, getCurrentNavigation has reverted to null by that point. The stackblitz above shows it does work in a component constructor, but is that guaranteed? If it is used inside a service subscription in the constructor for example, will it have reverted to null by the time it is called?

In general, is this approach supposed to replace the paramsMap.get method of passing params? I see it linked to a lot of those issues but the actual code here seems more related to the nav lifecycle hooks than giving the component access to the state?

@Airblader
Copy link
Contributor

In general, is this approach supposed to replace the paramsMap.get method of passing params?

I'm not from the Angular team, but I'd confidently say »No«. This additional state has nothing to do with route or query parameters, and accessing those is always possible from an ActivatedRoute (even outside a navigation).

The stackblitz above shows it does work in a component constructor, but is that guaranteed?

This is definitely an interesting question. I can't find any kindd of documentation or explanation as to what the design of on-going navigations and their lifecycle is. It's just a "it's on-going until it finished", which isn't very satisfying.

What I can tell you is where I already use this feature: I subscribe to query parameters and inside that subscription I access the current navigation to check the state for a specific flag.

@colmben
Copy link

colmben commented Mar 28, 2019

In general, is this approach supposed to replace the paramsMap.get method of passing params?

I'm not from the Angular team, but I'd confidently say »No«. This additional state has nothing to do with route or query parameters, and accessing those is always possible from an ActivatedRoute (even outside a navigation).

By paramsMap.get I more meant the optional params that are passed via the matrix method in routing. This new functionality here is already being touted on StackOverflow as the new method for doing data passing via routing:

https://stackoverflow.com/a/54365098/1669024

I can see why people would jump on it for optional params as it means they aren't getting tacked onto the URL (and are available to all segments of the route, not just the last as with the matrix method).

@Airblader
Copy link
Contributor

Airblader commented Mar 28, 2019

I can see why people would jump on it for optional params as it means they aren't getting tacked onto the URL

Yes, for that state would be a solution*. I'd question whether you want to hide it from the URL, though, since encoding it on the URL makes the link reentrant (but I'm biased).

*) As with everything, I'd evaluate it on a case-by-case basis, though. Personally I wouldn't go around and using history state for just about anything now. There's also implications to be considered such as state being kept when navigating back etc.

@colmben
Copy link

colmben commented Mar 28, 2019

Yes, for that state would be a solution. I'd question whether you want to hide it from the URL, though, since encoding it on the URL makes the link reentrant (but I'm biased).

And indeed that comes up in the SO comments :) In general though I think people are not liking those massive URLs...

@sagarjaspal
Copy link

Why is this only implemented for the RouterLinkWithHref-directive. So currently it's not possible to use the state-feature with a routerlink-directive at - let's say - a button.

I tried it with mat-card, didn't work :(

@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker area: router cla: yes target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow to pass data with router.navigate Router: Support Setting State Object When Navigating