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

Allow setting `history.state` when navigating #27198

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
@jasonaden
Copy link
Contributor

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

@googlebot googlebot added the cla: yes label Nov 20, 2018

@jasonaden jasonaden changed the title Fw 613 add data to events Allow setting `history.state` when navigating Nov 20, 2018

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Nov 20, 2018

@jasonaden jasonaden force-pushed the jasonaden:FW-613_add_data_to_events branch 3 times, most recently from 8859de5 to e78a000 Nov 29, 2018

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Nov 29, 2018

Show resolved Hide resolved packages/router/src/router.ts Outdated
Show resolved Hide resolved packages/router/src/router.ts
Show resolved Hide resolved packages/router/src/router.ts Outdated

@jasonaden jasonaden force-pushed the jasonaden:FW-613_add_data_to_events branch from 0fd82ac to 774916e Nov 29, 2018

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Nov 29, 2018

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Nov 29, 2018

@jasonaden jasonaden force-pushed the jasonaden:FW-613_add_data_to_events branch from 774916e to 45ee235 Nov 29, 2018

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Nov 29, 2018

@jasonaden

This comment has been minimized.

Copy link
Contributor Author

jasonaden commented Nov 29, 2018

@alxhub

This comment has been minimized.

Copy link
Contributor

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.

@alxhub

alxhub approved these changes Nov 29, 2018

Show resolved Hide resolved packages/router/src/events.ts Outdated
Show resolved Hide resolved packages/router/src/events.ts Outdated
Show resolved Hide resolved packages/router/src/events.ts Outdated
Show resolved Hide resolved packages/router/src/router.ts Outdated
Show resolved Hide resolved packages/router/src/router.ts Outdated
Show resolved Hide resolved packages/router/src/router.ts Outdated
* Previously successful Transition object. Only a single previous transition is available,
* therefore this previous Transition will always have a `null` value for `previousTransition`.
*/
previousTransition: Transition | null;

This comment has been minimized.

@IgorMinar

IgorMinar Nov 29, 2018

Member

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?

This comment has been minimized.

@jasonaden

jasonaden Nov 29, 2018

Author Contributor

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.

Show resolved Hide resolved tools/public_api_guard/router/router.d.ts Outdated
Show resolved Hide resolved tools/public_api_guard/router/router.d.ts Outdated
Show resolved Hide resolved tools/public_api_guard/router/router.d.ts Outdated

@jasonaden jasonaden force-pushed the jasonaden:FW-613_add_data_to_events branch 2 times, most recently from bf4ca6b to 8dd3a72 Nov 29, 2018

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Nov 29, 2018

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Nov 29, 2018

@jasonaden jasonaden force-pushed the jasonaden:FW-613_add_data_to_events branch from 8dd3a72 to 5ac9f5a Nov 29, 2018

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Nov 29, 2018

jasonaden added some commits Nov 14, 2018

feat(router): allow passing `state` to routerLink directives
This value will get written to the `history.state` entry.

FW-613 (related)
Related to #24617
feat(router): allow passing state to `NavigationExtras`
This value will get written to the `history.state` entry.

FW-613 (related)
@StevenLiekens

This comment has been minimized.

Copy link
Contributor

StevenLiekens commented Dec 4, 2018

Thanks, I've been waiting for this. Appreciate the effort.

@trotyl

This comment has been minimized.

Copy link
Contributor

trotyl commented Dec 10, 2018

@jasonaden Should scroll positions also be stored on history state based on this feature? Current object map is technically a memory leak that grows with navigations.

@Virendra-Yadav

This comment has been minimized.

@StevenLiekens

This comment has been minimized.

Copy link
Contributor

StevenLiekens commented Dec 11, 2018

I don't pretend to be a moderator but that's like the fifth time you advertised your package instead of commenting on the issue. Stop it.
https://github.com/angular/angular/issues?q=commenter%3AVirendra-Yadav

@jasonaden

This comment has been minimized.

Copy link
Contributor Author

jasonaden commented Dec 11, 2018

@trotyl It probably should be. I have an internal issue (just created this issue to track it) to deprecate navigationId in history.state. Then we will have a key we can store Angular-specific data under. At that point, yes, we can get scroll positions in there. This would have the advantage of also persisting scroll position through navigations.

If anyone comes across this and wants to work on the issue, that would be great. I should be getting to it within a couple weeks if not.

@Airblader

This comment has been minimized.

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

This comment has been minimized.

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!

FrederikSchlemmer added a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019

FrederikSchlemmer added a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019

feat(router): allow passing state to `NavigationExtras` (angular#27198)
This value will get written to the `history.state` entry.

FW-613 (related)

PR Close angular#27198

FrederikSchlemmer added a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019

feat(router): allow passing `state` to routerLink directives (angular…
…#27198)

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

FW-613 (related)
Related to angular#24617

PR Close angular#27198

FrederikSchlemmer added a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019

feat(router): add a Navigation type available during navigation (angu…
…lar#27198)

Provides target URLs, Navigation, and `NavigationExtras` data.

FW-613

PR Close angular#27198
@fjauer

This comment has been minimized.

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

This comment has been minimized.

Copy link

sky10p commented Jan 10, 2019

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

Thanks for your time!

@alecvolo

This comment has been minimized.

Copy link

alecvolo commented Jan 12, 2019

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

This comment has been minimized.

Copy link
Contributor

Airblader commented Jan 13, 2019

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

@f-aubert

This comment has been minimized.

Copy link

f-aubert commented Jan 22, 2019

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

This comment has been minimized.

Copy link
Contributor

Airblader commented Jan 22, 2019

@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

This comment has been minimized.

Copy link

f-aubert commented Jan 22, 2019

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.

ngfelixl added a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019

ngfelixl added a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019

feat(router): allow passing state to `NavigationExtras` (angular#27198)
This value will get written to the `history.state` entry.

FW-613 (related)

PR Close angular#27198

ngfelixl added a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019

feat(router): allow passing `state` to routerLink directives (angular…
…#27198)

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

FW-613 (related)
Related to angular#24617

PR Close angular#27198

ngfelixl added a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019

feat(router): add a Navigation type available during navigation (angu…
…lar#27198)

Provides target URLs, Navigation, and `NavigationExtras` data.

FW-613

PR Close angular#27198
@nhays89

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

jasonaden commented Mar 21, 2019

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.