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

[Router] RouterCancel #542

Closed
gfeller opened this issue Aug 14, 2018 · 16 comments
Closed

[Router] RouterCancel #542

gfeller opened this issue Aug 14, 2018 · 16 comments

Comments

@gfeller
Copy link

gfeller commented Aug 14, 2018

Versions

* ngxs: 3.2.0
* @angular/core: 6.0.3

Observed behavior

After "[Router] RouterCancel" the router state is not in sync with the current route:

image

The RouterCancel does not revert the state:
routercancelt

Desired behavior

The "router"-state should be in sync with the current route.

@FortinFred
Copy link
Contributor

FortinFred commented Aug 17, 2018

Have the same issue. You can reproduce it by adding a canDeactivate Guard that return false on a givin route.

I think the RouterCancel and RouterError should be handled differently in the State.


  @Action([RouterNavigation, RouterError, RouterCancel])
  angularRouterAction(ctx: StateContext<RouterStateModel>, action: RouterAction<any, RouterStateSnapshot>) {
    ctx.setState({
      ...ctx.getState(),
      state: action.routerState,
      navigationId: action.event.id
    });
  }

@FortinFred
Copy link
Contributor

FortinFred commented Aug 17, 2018

I also wonder if the RouteNavigation should be cancelled when a route guard prevents the navigation so that an ActionHandler would be able to handle the following:

this.actions$.pipe(ofActionCanceled(RouterNavigation)).subscribe(...);
Otherwise any processing on a RouteNavigation, has to be undoned on a RouteCancel.

RouterPlugin actions should cover the actions lifecycle or have the following actions:

  • RouterNavigation
    followed by one of the following:
  • RouterEnd
  • RouterCancel
  • RouterError

//Something in the lines of...  Have to set myself up for my first PR :)
  private setUpStateRollbackEvents(): void {
    this._router.events.subscribe(e => {
      if (e instanceof RoutesRecognized) {
        this.lastRoutesRecognized = e;
      } else if (e instanceof NavigationCancel) {
        this.dispatchRouterCancel(e);
      } else if (e instanceof NavigationError) {
        this.dispatchRouterError(e);
      } else if (e instanceof NavigationEnd) {
        this.dispatchRouterEnd(...);
      }
    });
  }

@matulkum
Copy link

Just ran into the same issue. This bug makes this plugin pretty much incompatible with route guards :(

romainZiba pushed a commit to romainZiba/opensportmanagement-angular that referenced this issue Nov 8, 2018
@xmlking
Copy link
Contributor

xmlking commented Nov 12, 2018

similar issue : #658

xmlking added a commit to xmlking/ngx-starter-kit that referenced this issue Nov 18, 2018
xmlking added a commit to xmlking/ngx-starter-kit that referenced this issue Nov 18, 2018
@xmlking
Copy link
Contributor

xmlking commented Nov 18, 2018

@deebloo @ngxs/router-plugin is unusable when using with Guards.

also we have types issue:
example, we are fourced to cast RouterStateSnapshot to any to use custom type RouterStateData I used in CustomRouterStateSerializer

    this.actions$
      .pipe(
        ofActionSuccessful(RouterNavigation),
        map((action: RouterNavigation) => action.routerState as any),
        distinctUntilChanged((previous: RouterStateData, current: RouterStateData) => {
          return previous.url === current.url;
        }),
      )
      .subscribe(data => {
        this.pageTitle.setTitle(data.breadcrumbs);
        this.analytics.setPage(data.url);
      });

https://github.com/xmlking/ngx-starter-kit/blob/develop/libs/core/src/lib/state/eventbus.ts#L74

this looks like a solution for type issues:
https://github.com/ngrx/platform/blob/master/modules/router-store/src/serializer.ts#L12

wonder if anyone working on fixing this plugin. if any help needed I can contribute.

@markwhitfeld
Copy link
Member

@xmlking Whould you like to try submit a PR for this issue?

@markwhitfeld markwhitfeld added this to the 3.4.x milestone Mar 11, 2019
@markwhitfeld
Copy link
Member

@gfeller This issue may have been fixed by PR #895 which has just been released in v3.4.3!
Could you test and confirm if this issue has been fixed or not?

@thomas-ama
Copy link

thomas-ama commented Mar 21, 2019

Im not sure it's linked to this bug but:

  • I type URL 1 in the address bar. The page renders. A RouterNavigation action is logged.
  • In the same tab, I replace URL 1 by URL 2 then hit enter. URL 2 begins to loads then a redirection occurs and URL 1 is rendered again. RouterCancel action (with routerState payload undefined) then RouterNavigation action are logged.

I'm using v3.4.3.

Edit: The bug is only in dev, it disappears in prod.

@splincode splincode modified the milestones: 3.4.x, 3.5.0 May 18, 2019
@markwhitfeld markwhitfeld modified the milestones: 3.5.0, vNext Jul 23, 2019
@markwhitfeld
Copy link
Member

@gfeller @thomas-ama could you test with the latest NGXS v3.5.0 and confirm if the issue still exists?

@qstiegler
Copy link

@gfeller @thomas-ama could you test with the latest NGXS v3.5.0 and confirm if the issue still exists?

I was just testing it and in dev it still happens.

@troydietz
Copy link
Contributor

Sadly, I'm seeing this on dev and prod

@markwhitfeld
Copy link
Member

@qstiegler @troydietz (and any others) would it be possible to test if this is fixed with the latest @dev version. I have just merged the above PR and it will be deployed to the @dev version by our CI in a few minutes. This PR should fix this issue.

@kpapke
Copy link

kpapke commented Aug 19, 2019

Hi, I was working with @troydietz when he reported our issue. I tested with the latest @dev version of NGXS router plugin and could not reproduce. Thanks for the quick fix!

@qstiegler
Copy link

@markwhitfeld Fix looks good for me. Thanks for your effort!

@arturovt
Copy link
Member

Thank you for your time guys, I appreciate that. Now I'm sure we can deliver that fix.

@markwhitfeld
Copy link
Member

Fixed in the v3.5.1 release! Closing this issue.

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

No branches or pull requests