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

First-class "client-side redirect" support #124

Open
domenic opened this issue May 27, 2021 · 5 comments
Open

First-class "client-side redirect" support #124

domenic opened this issue May 27, 2021 · 5 comments
Labels
addition A proposed addition which could be added later without impacting the rest of the API

Comments

@domenic
Copy link
Collaborator

domenic commented May 27, 2021

The scenario shown in #121 uses navigation.navigate(newURL, { replace: true }) in the middle of a navigate handler, to perform a "client-side redirect".

However, this is not optimal, as doing so makes the original navigation count as interrupted, and thus a "failure", with an "AbortError" DOMException as the failure reason. Notably it fires navigateerror on navigation, rejects navigation.transition.finished, and rejects the promise returned by navigation.navigate() if the navigation started that way.

There may be other downsides, e.g. the very-related discussion in #5 talks about the problems with losing same-siteness (for Sec-Same-Site) and form data. However, for purely client-side redirects I don't think those particular concerns apply. I guess maybe losing the userInitiated bit is in this vein, though.

We may want to contemplate a first-class redirect API, perhaps on the NavigateEvent object. The semantics would be something like a replace, but with the additional pieces:

  • navigation.transition.finished delays settling until the new navigation settles
  • navigation.navigate()'s return value delays settling until the new navigation settles
  • navigation.transition.rollback() during the new navigation rolls back both navigations.
  • event.signal for the original event does not fire abort.
  • Maybe we carry over event.userInitiated
  • Maybe we carry over event.info and event.formData unless told not to?
  • There is still a period where navigation.curentEntry is the intermediate page (e.g. if you're navigating to /b and want to redirect to /c, navigation.currentEntry will briefly be on /b before eventually replacing with /c). There's no way of getting around that.
@domenic
Copy link
Collaborator Author

domenic commented Jun 28, 2021

I just realized something that ups the priority here.

If you're synchronously trying to do the redirect, as in this example:

e.transitionWhile((async () => {
  if (shouldRedirect()) {
    await navigation.navigate("/redirected"); // (*)
  } else {
    // ...
  }
})());

then you should not use { replace: true }, since at the time the (*) line is running, the navigation hasn't happened yet: we're still in the synchronous phase of event dispatch (despite being inside an async function).

Whereas if you're asynchronously trying to do a redirect, as in this example

e.transitionWhile((async () => {
  if (await shouldRedirect()) {               // Note the await
    await navigation.navigate("/redirected"); // (*)
  } else {
    // ...
  }
})());

then you do need to use { replace: true }, because by that point the navigation has gone through and you're on the page that needs to get replaced.

This seems error-prone and fragile, so adding a first-class helper to smooth this over would be a good idea.

@domenic domenic added the addition A proposed addition which could be added later without impacting the rest of the API label Aug 23, 2021
@domenic
Copy link
Collaborator Author

domenic commented Jan 12, 2022

If you're synchronously trying to do the redirect, as in this example:

This example also causes annoying uncaught exceptions, since the first navigation being canceled causes transitionWhile() to throw an exception (since it's called on a canceled event). See https://crbug.com/1224413.

domenic added a commit that referenced this issue Apr 12, 2022
Closes #210. Discusses #124 in the main README now.
domenic added a commit that referenced this issue Apr 12, 2022
Closes #210. Discusses #124 in the main README now.
@atscott
Copy link
Contributor

atscott commented Apr 23, 2022

A concrete use-case for this might be for SPA app initial navigation. When going directly to a guarded URL, that navigation might be rejected. If the application developer wants to deny the the navigation but doesn't provide a redirect, how should the framework's router handle this? Right now, this can result in "blank" screens on initial page load.

Let's say the framework's router allows the application developer to specify a fallback to redirect to if the initial navigation fails, for example the home page. It would also be interested in tracking any redirects associated with that initial navigation so it doesn't eagerly navigate to the home page fallback when it should instead allow any associated redirects to complete.

Edit: would the first class redirect support multiple redirects? i.e., navigate to a-> redirects to b -> redirects to c

@domenic
Copy link
Collaborator Author

domenic commented Apr 25, 2022

It would also be interested in tracking any redirects associated with that initial navigation so it doesn't eagerly navigate to the home page fallback when it should instead allow any associated redirects to complete.

I don't quite understand that part; which redirects exist in this setup, besides the one from a guarded URL to the fallback homepage URL?

Edit: would the first class redirect support multiple redirects? i.e., navigate to a-> redirects to b -> redirects to c

Yes, I think that's the ideal.


To summarize my current thinking of what we could do for redirects: navigation.transition.redirect(url, { info }) would have the following impacts:

  • If the navigate event has not finished firing:
    • Cancel the navigate event (like preventDefault()), but without firing navigateerror or rejecting any promises or firing abort on the AbortSignal.
    • Start a new navigation, similar to navigation.navigate(url, { info }).
    • Update navigation.transition in place to track that navigation, instead of replacing the object like navigation.navigate() normally would. (This ensures anyone watching navigation.transition.finished gets notified at the right time.)
    • Make any { committed, finished } promises returned from any original navigation tie themselves to the new navigation.
  • Otherwise, if the navigate event has finished firing but we're still in the transition phase:
    • Start a new navigation, similar to navigation.navigate(url, { info, history: "replace" }). (Note the replace!!) Similarly don't fire navigateerror or reject any promises or fire abort, even though we are interrupting a transitioning navigation.
    • Update navigation.transition in place to track that navigation. I think this includes updating navigation.transition.navigationType (to replace) and navigation.transition.from (to the being-replaced intermediate entry).
    • Update any { finished } promise returned from the original navigation, similar to the above. (But we cannot update committed since it already settled.)
  • Otherwise, immediately reject. (This can only happen if you stored navigation.transition somewhere and called storedTransition.redirect(...) later; the transition property will get nulled out after the transition phase.)

All of these steps should work if done more than once.

@atscott
Copy link
Contributor

atscott commented Apr 25, 2022

It would also be interested in tracking any redirects associated with that initial navigation so it doesn't eagerly navigate to the home page fallback when it should instead allow any associated redirects to complete.

I don't quite understand that part; which redirects exist in this setup, besides the one from a guarded URL to the fallback homepage URL?

@domenic The fallback URL would only want to be applied if the initial navigation totally failed. There might be user-defined redirects associated with that navigation and wouldn't want to perform the fallback navigation if a different redirect succeeded. I believe the properties of the navigation you just outlined would make this task easy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition A proposed addition which could be added later without impacting the rest of the API
Projects
None yet
Development

No branches or pull requests

2 participants