Skip to content

event.redirect() doesn't trigger the navigate event #280

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

Open
posva opened this issue Feb 27, 2025 · 6 comments
Open

event.redirect() doesn't trigger the navigate event #280

posva opened this issue Feb 27, 2025 · 6 comments

Comments

@posva
Copy link
Contributor

posva commented Feb 27, 2025

The current version of event.redirect() only allows to change (once) the final committed URL in an intercepted navigation. In SPA routers, navigation guards can redirect multiple times 1. Each redirection goes through navigation guards and can be redirected again. It's up to the developer to ensure no infinite loops (the router can help detect such cases during development).

If event.redirect() only allows to change the URL before it is committed, it would not allow routers to intercept redirections created by users and force them to only use router redirections (inside navigation guards) so that the router can create a consistent experience. This is the current situation with the History API, the router has to take over and this limits users to not use the History API directly. In practice this affects any plugin that interacts with History API because it needs to be specific to a framework router. It also affects micro frontends in general because they struggle to coordinate different framework routers (they have to pause them based on the page).

Another possible solution is to treat new redirections as a full new navigation2, but this would still mean to discourage users from using event.redirect().

Footnotes

  1. At least for Angular and Vue:

  2. https://github.com/WICG/navigation-api/issues/124#issuecomment-2684371423

@atscott
Copy link
Contributor

atscott commented Feb 27, 2025

The current version of event.redirect() only allows to change (once) the final committed URL in an intercepted navigation.

Ah, that seems unexpected. I agree that there shouldn't be any limit on the redirect but I'm guessing this restriction might have been a safeguard against multiple different interceptors attempting to redirect since the current design proposal doesn't make the redirect observable from outside the place that called redirect().

If event.redirect() only allows to change the URL before it is committed, it would not allow routers to intercept redirections created by users and force them to only use router redirections (inside navigation guards) so that the router can create a consistent experience. This is the current situation with the History API, the router has to take over and this limits users to not use the History API directly. In practice this affects any plugin that interacts with History API because it needs to be specific to a framework router. It also affects micro frontends in general because they struggle to coordinate different framework routers (they have to pause them based on the page).
...
but this would still mean to discourage users from using event.redirect().

Navigations could be initiated from outside the router APIs as long as the finished/committed promises carry over across redirects. Are you also expecting multiple active participants in the navigation that might want to separately call commit, redirect, or reject the navigation? I can more easily imagine multiple interceptors but having all but one ultimately choose not to participate in the the navigation because the target URL is outside the realm of its defined routes. I struggle a bit more figuring out how things work if multiple interceptors/routers want to truly participate in a NavigateEvent and potentially reject or redirect a navigation after a different one has already processed its whole pipeline, committed, and activated its new routes and/or destroyed the old ones.

Angular does have UrlHandlingStrategy which was developed for migrating from AngularJS and allows you to outline how two routers on the same page should participate in a navigation. Here's a test where one router doesn't participate in changes to a particular part of the URL, theoretically something you could do to isolate one MFEs routing from another: https://github.com/angular/angular/blob/main/packages/router/test/integration.spec.ts#L7426-L7460. I've found it really hard (impossible, really) to reconcile what should happen when one participator thinks a navigation should succeed and the other thinks it should be rejected. I think this was at least part of the root cause for the CanDeactivate guards clobbering history when they reject a traversal because you can't just traverse back if you weren't handling the whole URL. Really you should only reset the portion of the URL path that you're participating in.

In SPAs routers, when a navigation guard redirect, that new redirection goes through navigation guards again and can redirect as many times as needed. 1

Would you want to pass the NavigateEvent from one SPA navigation to the next for the redirect? Without doing this, it would be harder for the SPA router to link one navigation to the following redirecting one. It seems more complicated to handle the redirect if you have to first call redirect and then wait for the new NavigateEvent and then link this to the redirecting navigation. I would imagine that even if the redirect behavior were to match the initial proposal, you might still want to first do the SPA redirect, passing the NavigateEvent to the next one, call redirect there, and then wait for the new NavigateEvent before proceeding.

As mentioned in #124 (comment), I think not being able to handle redirect this way would mean discouraging the use of event.redirect() in intercepted navigation when they use an SPA router and limiting the extensibility of the Navigation API. 1

Though I still have concerns what it looks like to have something outside the SPA router also be participating in the outcome of the navigation, would another possible solution be to have an event that makes the redirect observable to anyone that intercepted the original NavigateEvent (e.g. NavigateEvent.addEventListener('redirect', (e) => ...))? I don't know that it has to be represented as a new navigate event since it's really an attempt to modify the original navigation. IMO, this would be easier to handle than needing to figure out how to swap out to a new navigate event for a redirect and somehow determine that it's a redirect rather than a new navigation that aborted the previous one.

Footnotes

  1. https://github.com/WICG/navigation-api/issues/124#issuecomment-2687278792 2

@atscott
Copy link
Contributor

atscott commented Feb 27, 2025

I guess what I'm starting to think with MFEs, multiple routers on the page, or multiple individual interceptors of a given NavigateEvent is that there should probably at least be a way for all of them to communicate that they agree on wanting to call commit. If one of them already passed that point and does its "successful navigation" assumptions by creating the new routed components but another interceptor later decides "oh no, actually I want to redirect or reject this navigation", then it's a big headache for rollbacks. So maybe there should be something similar to the "navigatesuccess" where all intercept handler promises need to resolve but for commits so the interceptors can wait for that event/signal before proceeding past the commit point.

@domenic
Copy link
Collaborator

domenic commented Feb 28, 2025

The current version of event.redirect() only allows to change (once) the final committed URL in an intercepted navigation.

Allowing redirect() to change the URL multiple times is very reasonable and we can do that. It's a very simple method that, after performing a bunch of checks, just changes navigateEvent.destination.url.


If event.redirect() only allows to change the URL before it is committed, it would not allow routers to intercept redirections created by users and force them to only use router redirections (inside navigation guards) so that the router can create a consistent experience.

Let me be sure I understand the scenario you are working on. You are trying to write a library, called a router, which has its own navigate handler. And you want to ensure that your library's navigate handler is always called.

But, you are worried about users of your library also having their own navigate handlers, and using navigateEvent.redirect(). This will change the URL. In such cases, depending on event listener subscription order, your library will not necessarily see the changed URL.

(You can try to ensure your navigate listener runs later than other listeners by using addEventListener() with { capture: true }. But this will only run later than non-capture listeners; if a user adds a navigate handler that also uses { capture: true }, then they could run later. And, of course, users could asynchronously delay before their navigate handler code calls navigateEvent.redirect().)

Is this understanding correct?

If so, I tend to agree that we haven't designed the API in such a way. We've generally assumed there's only one "main" navigate event listener that is in control of the app. Other cases where this goes poorly is if different navigate event handlers all call navigateEvent.intercept() with conflicting options. It can work if there's one "in control" navigate event listener, and a few "observe-only" ones. But if multiple are trying to be in control, it's easy to get unexpected results, so this scenario isn't one we've put much time into.

Is supporting this kind of scenario, where there are multiple competing "in control" navigate event listeners, something you anticipate being used? I'm unsure how you see this working: if a web developer is using a router library, why would they also be expecting to have an "in control" navigate listener? Perhaps you could supply us with some realistic demo code?

As Andrew mentions, I'm unsure what a solution to this would look like, even if we did think it was supported. So realistic code would help straighten that out.


I guess what I'm starting to think with MFEs, multiple routers on the page, or multiple individual interceptors of a given NavigateEvent is that there should probably at least be a way for all of them to communicate that they agree on wanting to call commit.

Right now, commit() lives on NavigateEvent, and represents commiting a navigation. Every navigate event listener shares the same NavigateEvent, and so everyone has the ability to commit the navigation.

Andrew and I discussed an alternate model, where instead we associate ability-to-commit with the handler passed to navigateEvent.intercept({ handler, commit: "after-transition" }). That is, every such handler needs to separately say "ready to commit", before the actual commit operation happens. This could look something like this:

navigation.addEventListener("navigate", e => {
  e.intercept({
    commit: "after-transition",
    handler(controller) {
      await delay(1000);
      controller.commit();
      await delay(2000);
    }
  })
});

navigation.addEventListener("navigate", e => {
  e.intercept({
    commit: "after-transition",
    handler(controller) {
      await delay(2000);
      controller.commit();
      await delay(500);
    }
  })
});

navigation.addEventListener("navigate", e => {
  e.intercept({
    commit: "immediate",  // this generates a console warning, and we stay with "after-transition" behavior
    handler(controller) {
      await delay(5000);

      // Attempting to call this would throw an exception:
      // this handler has not opted in to being one of the committers.
      // controller.commit();
    }
  })
});

In this scenario, commit would happen at 2000 ms, and finished would happen at 5000 milliseconds. This does make commit and finish more symmetrical, in combining signals from all the different navigate event listeners.

We could do this. It adds complexity, and per the above it's not clear how important supporting this kind of multiple-"in control"-navigate listeners scenario is. But if there's a compelling use case, we can consider it. Unlike the observing-multiple-redirects situation, here at least the design is pretty straightforward.

@noamr
Copy link
Contributor

noamr commented Mar 10, 2025

We've discussed this at length internally, and there is some alternate proposal that somewhat changes the design of after-transition.

The model is as such:

  • The object passed to intercept can have a precommitHandler callback alongside the existing handler callback
  • The precommit handlers always run before the ordinary handlers
  • A precommit handler can redirect. When it does, it restarts the sequence.
  • Precommit handlers, unlike ordinary handlers, run in sequence - the next one only runs after the previous one has resolved (*alternatively, they can have an AbortSignal but that seems more error-prone)

Handler code can look like this:

navigateEvent.intercept({
  precommitHandler: async controller => {
     if (someCondition) {
       controller.redirect({url: "other.html"});
       return;
     }

     if (somethingIsInvalid) {
          throw new Error("Abort this navigation");
     } 
  }
  handler: async () => { await respond_to_committed_navigation(); }
});

This model allows for some basic coordination between handlers, i.e. what is requested in the OP.

The pseudo-code for this looks like this:

let precommit_handler_queue = event.pre_commit_handlers;
async function handle_event(event) {
  let restart = false;
  while (next_precommit_handler = precommit_handler_queue.pop()) {
    await next_precommit_handler({redirect: url => {
       event.destination.url = url;
       restart = true;
    });

    if (restart) {
       precommit_handler_queue = event.pre_commit_handlers;
    }
  }

  commit();
  await Promise.all(event.post_commit_handlers);
}

@posva
Copy link
Contributor Author

posva commented Mar 13, 2025

I apologize for taking so long to reply. Also I'm realizing now that I wasn't clear with my questions, sorry about that 😓. I will provide more code examples in the future to improve.

Are you also expecting multiple active participants in the navigation that might want to separately call commit, redirect, or reject the navigation?

I now understand that it doesn't make sense for multiple people to call event.redirect() because the handlers run in parallel. In my case, I was thinking of using that method internally when users do a redirect through a router navigation guard. But as you mentioned, it's important to keep the same finished and committed promises, so I can't implement redirects that way.

Would you want to pass the NavigateEvent from one SPA navigation to the next for the redirect?

Currently, it's kind of a nested navigation. I think the code below explains it better.

Allowing redirect() to change the URL multiple times is very reasonable and we can do that. It's a very simple method that, after performing a bunch of checks, just changes navigateEvent.destination.url.

Sorry I wasn't clear. Redirecting multiple times sounds interesting. In vue router, a redirection immediately restarts all of the navigation guards while the returned promise from router.push() still resolves to the final (the redirection) navigation

router.beforeEach((to, from) => {
  console.log("first guard");
  if (to.path === "path" && iWanttoRedirect) {
    return "/redirected";
  }
});

// just for the demo, it only logs
router.beforeEach((to, from) => {
  console.log("second guard");
});

await router.push("/somewhere");
// first guard
// first guard
// second guard
router.currentRoute.path; // /redirected

In the current implementation, the to will first contain to.path === '/somewhere' while in the second one it will become /redirected and a new to.redirectedFrom property will point to the previous to. The from property stays the same in all cases.

Let me be sure I understand the scenario you are working on. You are trying to write a library, called a router, which has its own navigate handler. And you want to ensure that your library's navigate handler is always called.
...
Is this understanding correct?

I don't expect users to directly call event.redirect(). I expect them to use the router abstraction. But I was worried of what would happen if someone added a call in the code with a custom navigate interception. Maybe I overthinking it.

If so, I tend to agree that we haven't designed the API in such a way. We've generally assumed there's only one "main" navigate event listener that is in control of the app.

Thank you for this. Knowing the intended usage helps me a lot to drive the router's API to be in sync with the Navigation API. So I would assume other navigate events are more about observation and maybe delaying the navigation but not controlling it.

Is supporting this kind of scenario, where there are multiple competing "in control" navigate event listeners, something you anticipate being used?

I think not.


The precommitHandler looks interesting. I will reply to that in different comment.

@atscott
Copy link
Contributor

atscott commented Mar 13, 2025

In my case, I was thinking of using that method internally when users do a redirect through a router navigation guard. But as you mentioned, it's important to keep the same finished and committed promises, so I can't implement redirects that way.

I think using the redirect method would be what you want internally. That’s the way you keep the NavigateEvent promises from resolving or rejecting and retain a “continuation” of the initial event. Unless, like I realized below, you only call redirect once, right before commit.

Redirecting multiple times sounds interesting. In vue router, a redirection immediately restarts all of the navigation guards while the returned promise from router.push() still resolves to the final (the redirection) navigation

The Angular Router works much the same way in this. My first prototyping didn’t bother attempting to map Router redirects to a proper “redirect” in the browser navigation API and just started a fresh one since that somewhat mapped to the router events.

But after your comment I thought it might be better to use redirect and retain the original NavigateEvent so the committed and finished promises don’t reject and instead resolve after the final navigation. I suppose it could also hold off on calling redirect until right before the commit, once we’re sure no more redirects will happen in the Router. That might actually be easier to manage…I’ll have to think about it some more.

The precommitHandler looks interesting. I will reply to that in different comment.

Will be curious to hear your thoughts again and thank you for your feedback thus far! We definitely gave the multi-interceptor idea consideration here but I think precommit handler offers some niceties on its own:

Some things I like about this approach (mentioned at various times previously but now organized in one place):

  • redirect and commit only available where you can use it (in the precommit handler and not post-commit)
  • Commit function isn’t necessary since it’s done by resolving precommit handler, which matches well with the behavior of resolving the handler promise
  • Deferred commit specified implicitly by the separate precommit handler (no more “after-transition”)
  • (probably?) No more warnings about different interceptors using different commit strategies
  • Ability to more easily coordinate multiple intercept handlers. It’s still far from easy but maybe doable. Still a lot to explore there since routers couldn’t actually handle this before so it’s new territory and lots of complicated edge cases.

A couple potential downsides:

  • there’s now a little more back and forth between the NavigateEvent and the router. You can’t always proceed immediately but have to wait for your handler to execute.
  • post-commit redirects are off the table for now but if we wanted those in the future, I don’t know if there’s as good a place for them anymore (maybe the same controller.redirect idea for handler)
  • overall behavior of the api is more complicated and behaviors are split between two handlers. It requires more knowledge to know how things work. The timing and interaction between the handlers isn’t immediately obvious (Precommit handlers rerunning on redirect, the sequence of handlers, especially with multiple interceptors). As a reader, it was maybe easier to understand “event.redirect” and “event.commit”.

domenic pushed a commit that referenced this issue Mar 21, 2025
This removes the sequential and restarting behavior from them. See related discussion in #280.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants