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

Will the current transitionWhile() design of updating the URL/history entry immediately meet web developer needs? #66

Open
domenic opened this issue Mar 9, 2021 · 85 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 Mar 9, 2021

As discussed previously in #19 (and resolved in #46), intercepting a navigation using the navigate event and converting it into a same-document navigation will synchronously update location.href, navigation.currentEntry, and so on. I think this is on solid ground per those threads; delaying updates is problematic. (EDIT: further discussion, starting around #66 (comment), reveals this is not quite settled.)

However, I wonder about the user experience of when the browser URL bar should update. The most natural thing for implementations will be to have the URL bar match location.href, and update immediately. But is this the experience web developers want to give their users?

I worry that perhaps some single-page apps today update the URL bar (using history.pushState()) later in the single-page-navigation lifecycle, and that they think this is important. Such apps might not want to use the app history API, if doing so changed the user experience to update the URL bar sooner.

The explainer gestures at this with

TODO: should we give direct control over when the browser UI updates, in case developers want to update it later in the lifecycle after they're sure the navigation will be a success? Would it be OK to let the UI get out of sync with the history list?

and this is an issue to gather discussion and data on that question. It would be especially helpful if there were web application or framework authors which change the URL in a delayed fashion, and could explain their reasoning.

A counterpoint toward allowing this flexibility is that maybe standardizing on the experience of what SPA navs feel like would be better for users.

@domenic domenic added the addition A proposed addition which could be added later without impacting the rest of the API label Mar 9, 2021
@kenchris
Copy link

I will try to reach out to some of my framework friend and see if they will leave any feedback

@manekinekko
Copy link

Thank you @kenchris for sharing this.

@mhevery @IgorMinar @alxhub @mgechev @@pkozlowski-opensource how is this going to affect Angular?

@manekinekko
Copy link

Also adding my friend @posva from the Vue.js team. Any feedback on this, Eduardo?

@posva
Copy link
Contributor

posva commented Mar 22, 2021

I think this is on solid ground per those threads; delaying updates is problematic.

This is true. Being able to get the updated location right after pushState() in all browsers is easier to handle at framework level.

respondWith() design seems to be good to be able to use it. I would need to play with it to better know of course.

From Vue Router perspective, all navigations are asynchronous, meaning calling router.push('/some-location') will update the URL at least one tick later due to navigation guards (e.g. data fetching, user access rights, etc). This also applies for history.back() (or clicking the back buttons which seems to be the same) but in that scenario, the URL is updated immediately, and it's up to the router framework to rollback the URL if a navigation guard cancels the navigation. This means the URL changes twice in some cases (e.g. an API call to check for user-access rights). From what I understood with #32 (comment), this shouldn't be a problem anymore as we could avoid changing the URL until we are sure we can navigate there.

@atscott
Copy link
Contributor

atscott commented Mar 22, 2021

I don't have much additional to add for Angular beyond what @posva mentioned for Vue. Angular's router operates in much the same manner.

We do provide the ability to update the URL eagerly via the urlUpdateStrategy. The default is 'deferred' though, which means for most cases, the URL is update after guards run, like @posva mentioned for Vue.

We also have the same issue with the history.back/back buttons/url bar updates and we specifically don't do a good job of rolling back the URL (angular/angular#13586 (comment)) - this would be easy to fix with the app history API.

I'm in the process of going through the proposal and evaluating how we would integrate it into the Angular router.

@kenchris
Copy link

Hi there @atscott @posva can you explain me what those guards are and how they work? Asking for the @w3ctag

How long are these commonly deferred? Are we taking about microseconds here or can they be deferred indefinitely

@bathos
Copy link

bathos commented Apr 20, 2021

I worry that perhaps some single-page apps today update the URL bar (using history.pushState()) later in the single-page-navigation lifecycle, and that they think this is important. Such apps might not want to use the app history API, if doing so changed the user experience to update the URL bar sooner.

I’m in that camp, but it’s not about the URL bar for me, it’s about ensuring APIs like location.href and node.baseURI are in sync with the effective current state of the application. I’ve seen bugs arise during the brief time between the start and end of async resolution of a new application state because of components treating the current URL as a source of truth*. Since switching to unilaterally performing an initial URL rollback before async resolution a few years ago, I’ve never seen these problems again.

Since the URL bar’s contents aren’t accessible to JS, it’s the one aspect of this which doesn’t matter to me — whether it updates optimistically wouldn’t impact whether there is an observable misaligned transitional state.

(It seems I want the exact opposite of what’s been described, if I understand right? — cause for me it’s “not delaying updates [to location.href, etc] is problematic.” Immediately-update-code-observable-location behavior might prevent us from making meaningful use of respondWith, though other aspects of AppHistory would remain useful to us regardless.)

How long are these commonly deferred? Are we taking about microseconds here or can they be deferred indefinitely

While I can’t answer for the folks above, brief unscientific testing suggests that in our case it falls between 0 and 150ms (w/ navigator.connection.effectiveType 4g). The timing depends on what dependent API requests are involved, if any. It seems fair to describe it as some specific fraction of the time it would take to load a whole new document if we were doing SSR since it’s a subset of the same work that would be done if we were (fetch x from the database, etc).

* Ex: Relative query param links, e.g. href="?page=2" from templates associated with the “old state” (which generally would not be replaced until the route resolution completes) will link to the wrong thing during the transition.

@posva
Copy link
Contributor

posva commented Apr 20, 2021

Hi there @atscott @posva can you explain me what those guards are and how they work? Asking for the @w3ctag

How long are these commonly deferred? Are we taking about microseconds here or can they be deferred indefinitely

Sure, they are explained here and they usually last between 16ms or a few seconds. They can be deferred indefinitely but they shouldn't.

@bathos
Copy link

bathos commented Jun 28, 2021

Currently playing with the Chromium impl, the required pattern to achieve the behavior we currently have (location updates after successful state transition) is roughly:

  • on navigate, branch based on info value that may or may not be some sentinel:
    • sentinel absent and canRespond:
      • create & dispatch custom event (e.g. "navigaterequest"). associate orig destination / impl similar respondWith API
      • if custom respondWith called:
        • preventDefault / stopprop original event
        • await custom respondWith promise
          • if rejected, dispatch custom navigateerror (and do not commit the failed change to url)
          • otherwise, create navigate options based on original destination + promise result value (e.g. url canonicalization) and call navigate() with the sentinel info value to "commit" it

The built-in respondWith needs to be ignored in this model - the only "response" action is to preventDefault - but the end result is a similar API surface. It does seem like an improvement because it involves less code than current History-based solutions, but the misalignment is also very stark.

@domenic
Copy link
Collaborator Author

domenic commented Jun 28, 2021

Very interesting! The fact that you are able to mostly emulate the delayed-URL-update behavior based on the current primitives is a good sign. It feels similar to #124 in that maybe we can add it to the API on top of what exists, so that anyone who wants to achieve a similar experience can just use app history directly instead of needing to get all the edge cases right in a way similar to what you describe.

@bathos
Copy link

bathos commented Jul 28, 2021

Edit: This was a previously question I asked due to mistaking a behavior change related to location updates for one which would implement the behavior I’m seeking. But I got ahead of myself and can answer my own question about it: the bit I was clearly not paying attention to was that it said “after all handlers have finished running,” so no, the behavior did not change in the way I thought. Leaving the comment intact below to avoid confusion if people got emails.

Original comment/question

@domenic I just saw #94 (comment):

Change the updating of location.href and appHistory.current to synchronously happen after all handlers have finished running (if respondWith() was called by any of them), instead of happening synchronously as part of respondWith(). This helps avoid the problem where future handlers see appHistory.current === appHistory.destination. Edit: this was done in Move URL/history updating out of respondWith() #103.

The last of these is done. I'm interested in feedback on which, if any, of the first three of these we should do.

Is it correct to interpret this as saying event.methodCurrentlyKnownAsRespondWith(x) behavior has been changed to now defer the update of observable location/current until x settles?

Previous comments had given me the impression that location/current updating at the start rather than the end of a transition was a settled question. Our last two comments here are from a month later than the comment I quoted, too — so I’m not sure if maybe I’m misinterpreting what I read there.

If this behavior is set to change, the cancel-initially-but-repeat-the-navigation-again-later dance I described in my prior comment won’t be needed as far as I can tell. Since I think this was the only challenging disconnect between AppHistory’s behavior and the routing behavior we currently implement and want to preserve, if location update deferral is real now, it likely means we’ll be able to use the appHistory API “bare”!

I am dizzy imagining the possible thundering satisfication of just outright deleting ten thousand pounds of terrifying tower-of-hacks-and-monkey-patches History mediation code without leaving one trace ... gonna go lie down whew

@domenic
Copy link
Collaborator Author

domenic commented May 23, 2022

From @jakearchibald in #232

With MPA:

1. Begin fetching

2. Time passes

3. Fetch complete

4. Switch to new page - URL changes here

With transitionWhile:

1. Begin fetching

2. `transitionWhile` called - URL changes here

3. Time passes

4. Fetch complete

5. Alter DOM

This means old-page content is being shown between steps 2-5, but with the new URL. This presents issues for relative URLs (including links, images, fetch()), which won't behave as expected.

It feels like URL changing should be deferred until the promise passed to transitionWhile resolves, perhaps with an optional commitURLChange() method to perform the swap sooner.

Along with the URL issue, there's also the general issue of the content not matching the URL.

We've known for some time that the content will not match the URL. This thread (as well as various offline discussions) have various framework and router authors indicating that is OK-ish, except for @bathos who is working around it.

The novel information for me here is Jake's consideration for relative URLs. It's not clear to me exactly what goes wrong here; surely for new content, you want to fetch things relative to the new URL? But I can imagine maybe that's not always what's assumed.

Anyway, toward solutions: the big issue here is that any delay of the URL needs to be opt-in because it breaks previous platform invariants. Consider this:

navigation.addEventListener("navigate", e => {
  e.transitionWhile(delay(1000), { delayURLUpdate: true });
});

console.log(location.href); // "1"
location.hash = "#foo";
console.log(location.href); // Still "1"!!!

Similarly for all other same-document navigation APIs, like history.pushState(), which previously performed synchronous URL updates.

As long as we have such an opt-in, we can probably make this work. The page just has to be prepared for same-document navigation APIs to have delayed effects.

@jakearchibald
Copy link

jakearchibald commented May 23, 2022

navigation.addEventListener("navigate", e => {
  e.transitionWhile(delay(1000), { delayURLUpdate: true });
});

console.log(location.href); // "1"
location.hash = "#foo";
console.log(location.href); // Still "1"!!!

fwiw, I think it'd be fine if fragment changes are guaranteed to be synchronous. Only navigations that would be cross-document can have their URL update delayed, matching MPA behaviour.

@jakearchibald
Copy link

jakearchibald commented May 24, 2022

Here are a couple of demos that might help:

https://deploy-preview-13--http203-playlist.netlify.app/ - requires Canary and chrome://flags/#document-transition. There's a two second timeout between clicking a link and the DOM updating.

This was supposed to be my "works fine" demo because it doesn't use relative links. However, I noticed that if I click one thumbnail, then click another within the two seconds, the transition isn't quite right.

This is because my code is like:

navigation.addEventListener('navigate', (event) => {
  event.transitionWhile(
    createPageTransition({
      from: navigation.currentEntry.url,
      to: event.destination.url,
    })
  );
});

Because the URL commits before it's ready, on the second click the from doesn't represent the content. To work around this, I guess I'd have to store a currentContentURL somewhere in the app, and try to keep that in sync with the displayed content.

https://deploy-preview-14--http203-playlist.netlify.app/ - here's an example with relative URLs. If you click one thumbnail then another, you end up on a broken URL. Even if you click the same thumbnail twice, which some users will do accidentally.

My gut feeling is that relative URLs on links is uncommon these days, but there are other types of relative URL that will catch folks out here. Eg, fetching ./data.json and expecting it to be related to the content URL.

@jakearchibald
Copy link

jakearchibald commented May 24, 2022

I wonder if frameworks have adopted the model of:

  1. Update URL
  2. Fetch content for new view while old view remains
  3. Update DOM

…because of limitations in the history API, or whether they think this is the right model in general. Thoughts @posva @atscott?

It just seems like a fragile ordering due to things like relative URLs, and other logic that wants to do something meaningful with the 'current' URL, assuming it relates to the content somehow. It's also a different order to regular navigations, where the URL updates only once content is ready for the new view.

@jakearchibald
Copy link

sveltejs/kit#4070 - Svelte switched to a model where the URL is only changed when the navigation is going to succeed, as in the new content is ready to display.

From the issue:

This is arguably less good UX since it means a user may get no immediate feedback that navigation is occurring

This isn't a problem with the navigation API, as it activates the browser loading spinner.

@bathos
Copy link

bathos commented May 24, 2022

@jakearchibald FWIW, we don’t use relative pathnames, but we use relative query-only links in all our search/filter panels. This is where URL-first hit us.

@jakearchibald
Copy link

Oh yeah, I imagine that kind of thing is way more common

@Rich-Harris
Copy link

Echoing Jake, updating the URL before the navigation completes is very fragile. It's true that many prominent SPAs (including the very site on which we're having this discussion) do update the URL eagerly — and I suspect it is because there's otherwise no immediate feedback that the navigation intent was acknowledged — but we moved away from this approach in SvelteKit because it's buggy. Anything that involves a relative link during that limbo period is likely to break.

Here's a simple demo — from the / route, click 'a-relative-link` a few times. The app immediately gets into a broken state: https://stackblitz.com/edit/node-sgefhh?file=index.html.

Browser UX-wise, I think there's a case for updating the URL bar immediately — it provides an even clearer signal that activity is happening, and makes it easier to refresh if the site is slow to respond, and so on. But I think it's essential that location doesn't update until the navigation has completed. In fact I'd go so far as to say that updating location immediately could be a blocker for SvelteKit adopting the navigation API, which we very much want to do!

@domenic
Copy link
Collaborator Author

domenic commented May 24, 2022

Thanks all for the great discussion. My current take is that we need to offer both models (early-update and late-update/update-at-will), and this discussion ups the priority of offering that second model significantly. Read on for more philosophical musings...

Cross-document (MPA) navigations have essentially four key moments:

  • Start: clicking the link, setting location.href, etc.
  • Commit: after the fetch of the destination page has gotten back headers, unload/pagehide fire on the starting page, the URL bar updates, and the content of the starting page becomes non-interactive. (This point used to flash a white screen, but "paint holding" these days keeps the old content around, non-interactively.)
  • Switch: as response body content for the destination streams in, at some point it replaces the old page as the displayed content.
  • Finish: once the response body, and all blocking subresources, have finished loading, the load/pageshow events fire. (The actual user perception of "finish" is usually somewhere before this point, leading to metrics like "FCP", "LCP", etc. to try to capture the user experience.)

The raw tools provided by history.pushState() allow you to emulate a variety of flows for same-document (SPA) navigations. For example, to emulate the above, you would:

  • Start: Watch for click events on links. When they happen, start a fetch, and maybe add an in-page loading indicator.
  • Commit: When the fetch headers come back, make the current page non-interactive, and call history.pushState().
  • Switch: At some point during the fetch body streaming, swap in the fetched content into the document
  • Finish: Once the entire response body has streamed in and been rendered, stop any in-page loading spinner.

But you can also emulate other flows, e.g. many people on Twitter mention how they prefer to consolidate Start + Commit. Or, you can do things MPAs can't accomplish at all, such as replacing "Switch" with "Show Skeleton UI", and then doing Start + Commit + Show Skeleton UI all together at once.

The navigation API currently guides same-document (SPA) navigations toward the following moments:

  • Start+Commit: clicking a link, setting location.href, etc. will fire a navigate event. Then, the developer immediately updates location and the URL bar by calling navigateEvent.transitionWhile(). This starts the loading spinner, and creates the navigation.transition object.
  • Switch: at some developer-determined point, they can replace the content of the page with something from the network (or wherever).
  • Finish: at some developer-determined point, when the promise passed to navigateEvent.transitionWhile() settles, stop the loading spinner, set navigation.transition to null, and fire a navigatesuccess (or navigateerror) event.

We've found this to be pretty popular. Especially given how developers are keen to avoid double-side effect navigations (e.g. double submit), or to provide instant feedback by e.g. loading a skeleton UI of the next screen instead of waiting for network headers. We've even found Angular transitioning away from a model that allows late-Commit, into one that only allows Start+Commit coupled together. But as some folks on this thread are pointing out, it's not universal to couple those two. And coupling comes with its own problems; it's really unclear what the best point at which to update location is, with each possibility having tradeoffs depending on what assumptions your app and framework are making.

So we should work on some way of allowing Commit to be separated from Start, for SPAs and frameworks which want that experience.

@domenic
Copy link
Collaborator Author

domenic commented May 24, 2022

So, https://twitter.com/aerotwist/status/1529138938560008194 reminded me that traversals are very different here than push/replace/reload navigations. For implementation reasons, it is extremely difficult to allow delayed Commit for them. (This is in fact a big reason why Angular is transitioning to not allowing delayed Commit.)

Introducing an asymmetry between traversals and others seems unfortunate, so I'm now less optimistic about solving this soon...

We've discussed something similar in #32, about preventing traversal Commit. Preventing and delaying are essentially equivalent, because delaying means preventing and then re-starting. Some of the more technical discussions in #207 and #178. The best current plan is allowing preventing traversal Commit for top-level frames only, and only for navigations initiated by user activation.

How would you all developers/framework authors feel if we could only delay Commit for push/replace/reload, but traversals always had Start+Commit coupled?

@atscott
Copy link
Contributor

atscott commented May 24, 2022

I wonder if frameworks have adopted the model of:

  1. Update URL
  2. Fetch content for new view while old view remains
  3. Update DOM

…because of limitations in the history API, or whether they think this is the right model in general. Thoughts @posva @atscott?

It just seems like a fragile ordering due to things like relative URLs, and other logic that wants to do something meaningful with the 'current' URL, assuming it relates to the content somehow. It's also a different order to regular navigations, where the URL updates only once content is ready for the new view.

Here's the historical context for the eager vs deferred url updates in Angular: angular/angular#24616

In defense of the initial design deferring the URL update:

...application developers would want to stay on the most recently successfully navigated to route.

The motivation to also support URL eager updates:

However, this might not always be the case. For example, when a user lands on a page they don't have permissions for, we might want the application to show the target URL with a message saying the user doesn't have permissions. This way, if the user were to copy the URL or perform a refresh at this point, they would remain on the same page.

Angular supports both eager and deferred URL updates and will likely need to continue to do that. Having the support for both in the navigation API would make adopting it easier so we won't have to work as hard to get both behaviors.

FWIW, I've never felt totally comfortable with the deferred strategy because it's not possible to maintain this behavior for all cases. What I mean is that even when using the deferred URL update strategy, the URL update is still "eager" when users navigate with forward/back buttons or the URL bar directly.

On the issue with relative links: we don't exactly have this issue in Angular because we expect users to only trigger navigations using the router APIs (for better or for worse). Links in the application are expected to use the routerLink directive which captures the click event on a href, navigates with the router API, and prevents the browser from doing its default navigation (code reference)

@jakearchibald
Copy link

jakearchibald commented May 24, 2022

@domenic

For implementation reasons, it is extremely difficult to allow delayed Commit for them

Ah, that's a real sadness. They behave with delayed commit in regular navigations.

I agree that there's little point adding delayed commit for new navigations if we can't have them for traversals too.

@Rich-Harris
Copy link

How would you all developers/framework authors feel if we could only delay Commit for push/replace/reload, but traversals always had Start+Commit coupled?

That's how it works at present for those of us deferring pushState, so it would be acceptable but perhaps not ideal — it would be definitely be more robust if the meaning of relative links wasn't dependent on the type of navigation taking place.

I noticed that traversals do involve a delayed commit when bfcache is disabled (e.g. there's an unload event handler), which makes me wonder if it's truly an impossibility in the case where the Navigation API is being used. I'm also curious about the extent to which we're limited by location mirroring what's in the URL bar — is there a possible future in which the URL bar updates immediately (to satisfy whatever design constraints make delayed commit difficult) but location only updates once navigation completes, or is that a non-starter?

Small point of clarification: to use the terminology in #66 (comment), SvelteKit navigations (other than traversals) are Start then Commit+Switch+Finish. I think that's probably the case for most SPA-ish frameworks (i.e. there are two 'moments', and it's either that or Start+Commit then Switch+Finish); IIUC even frameworks that do Turbolinks-style replacements generally do a single element.innerHTML = html rather than something more granular involving streaming.

My current take is that we need to offer both models

I've spent a lot less time thinking about the nuances of the Navigation API than other people in this thread, but it does feel like something as fundamental as navigation should have one correct approach. The fact that SPA frameworks currently have different opinions about this is entirely because of the History API's terribleness, and I think it would be a shame if that prevented the Navigation API from being a clean break with the past.

On the issue with relative links: we don't exactly have this issue in Angular because we expect users to only trigger navigations using the router APIs (for better or for worse).

We're getting into edge case territory, but it's not just relative links — they're just the most obvious way in which things can break (and it's not always practical to use a framework-provided component, for example when the links are inside HTML delivered from a CMS). Constructing <link> elements, or images or iframes, or fetching data will also break if you're using relative URLs.

@domenic
Copy link
Collaborator Author

domenic commented May 24, 2022

I noticed that traversals do involve a delayed commit when bfcache is disabled (e.g. there's an unload event handler), which makes me wonder if it's truly an impossibility in the case where the Navigation API is being used.

Sort of. How it actually works is that traversals are locked in and uncancelable roughly from the moment they start. So from the browser's point of view, they are "committed". (But, I agree this is not precisely the same as the "Commit" step I mentioned above... I can dig into this in nauseating detail if we need to.)

In more detail, there is an inter-process communication sent immediately upon any traversal-start, which gets a lot of machinery irreversably moving, including (in the non-bfcache case) the network fetch. So the technical problems come with inventing a new code path which delays this usual flow.

There is some precedent, in that beforeunload can prevent traversals. But then you start getting into less-technical issues around preventing back-trapping abuse...

Anyway, to be clear, I'm saying "extremely difficult" and "less optimistic about solving this soon", not "impossible".

I'm also curious about the extent to which we're limited by location mirroring what's in the URL bar — is there a possible future in which the URL bar updates immediately (to satisfy whatever design constraints make delayed commit difficult) but location only updates once navigation completes, or is that a non-starter?

It seems possible in theory to decouple them, subject to security/UI team's review. However I'm not sure how many problems it solves. The technical design constraints are not really related to the UI. On the implementation-difficulty side, they're as explained above; on the web developer expectations side, I think they're about what a given app/framework's expectations are for location, and I think we have examples of both expectations.

I think that's probably the case for most SPA-ish frameworks (i.e. there are two 'moments', and it's either that or Start+Commit then Switch+Finish);

I agree most SPA-ish frameworks tend to prefer a two-moment model. However I most commonly experience a split of Start+Commit+Switch, then Finish. E.g. start on a fresh tab with https://twitter.com/home , then click the "Explore" button.

I've spent a lot less time thinking about the nuances of the Navigation API than other people in this thread, but it does feel like something as fundamental as navigation should have one correct approach. The fact that SPA frameworks currently have different opinions about this is entirely because of the History API's terribleness, and I think it would be a shame if that prevented the Navigation API from being a clean break with the past.

I'm really torn on this!! And have been since March 9, 2021, when this thread was opened :).

My instincts are very similar to what you're saying. However I just have a hard time picking a model and saying everyone should use the same one. Especially given all the difficulties around traversals, and how I thought we had a good model but now Jake and you are coming in with solid arguments for a different model.

I think the constraints are the following:

  • Switch needs to be developer-controlled since it's decoupled from navigation API concerns; it's really about presentation. Unless we tie the navigation API really strongly to some DOM-update or transition model, we can't enforce where it is grouped.
  • We can either couple Start+Commit, or separate them. Separating them is very hard for traversals.

So this leads to the navigation API providing either Start+Commit, Switch, Finish; Start, Commit, Switch, Finish; or Start, Switch, Commit+Finish. Here + means inherently-coupled, and , means they are separate steps which the web developer can couple or decouple as they want.

The fact that I am torn and people have good arguments for both places to put Commit, makes me want to build Start, Commit, Switch, Finish. But if everyone was willing to accept Start+Commit, Switch, Finish, then we could say that's canonical today, and do no further work. And if everyone was willing to accept Start, Switch, Commit+Finish, then we could try to put the brakes on this API, go back to the drawing board, and try to build that. But I don't see that kind of agreement...

Constructing <link> elements, or images or iframes, or fetching data will also break if you're using relative URLs.

I am really curious to hear more about how people handle this. Jake and I were unable to find any non-demo sites that were broken in the wild by this issue. This suggests developers are either working around it painfully, or their frameworks are handling it for them. E.g., one way a framework could handle it for you is by preventing any code related to the "old" page from running, after Commit time. Is that a strategy anyone has seen? What does Angular do, @atscott?

@atscott
Copy link
Contributor

atscott commented May 24, 2022

This suggests developers are either working around it painfully, or their frameworks are handling it for them. E.g., one way a framework could handle it for you is by preventing any code related to the "old" page from running, after Commit time. Is that a strategy anyone has seen? What does Angular do, @atscott?

I don't know if I totally follow the whole problem here but I think the missing piece from @Rich-Harris is "it's not always practical to use a framework-provided component, for example when the links are inside HTML delivered from a CMS". To me, this means we're working outside the confines of the framework so this isn't really anything Angular would or could control.

For things inside the application, we do expect any URLs to be constructed from Router APIs. This means that any relative links created from within the application would only update when the Angular Router state updates. That said, regardless of "eager" or "deferred" browser URL/location updates, this internal state update in the Router is always deferred until we are pretty certain the activation will happen: https://github.com/angular/angular/blob/8629f2d0af0bcd36f474a4758a01a61d48ac69f1/packages/router/src/router.ts#L917-L927

@natechapin
Copy link
Collaborator

As far as what scroll() does before commit(), in my opinion there's only two real options:

  1. Throw an error
  2. Behave equivalently as it does after commit(), and attempt to scroll to either the current hash or top of the page.

I'd personally lean toward option 1. For one thing, if the user had scrolled the entry being navigated away from, invoking scroll() would clobber the user scroll position, which seems unfortunately.

Also, we currently only allow one scroll() per navigate event. I'd be hesitant to allow that to be consumed pre-commit.

@tbondwilkinson
Copy link
Contributor

Ah I missed the part about limiting one scroll() per navigate. Then yeah, I'd agree, throwing an Error is probably a better feedback option to users

@natechapin
Copy link
Collaborator

@natechapin

My work-in-progress prototype captures scroll position during commit (i.e., precommit handlers can affect the scroll position of the previous history entry before navigating away from it, but post commit handlers affect the destination entry).

I really like this model. "commit at start" vs "commit at end" as a switch isn't detailed enough, as generally you'll want to commit sometime between the two (along with the first DOM change).

But, to avoid the two callbacks, how do you feel about this:

navigateEvent.intercept({
  commit: "manual",
  async handler() {
    // The old URL is still active.
    const dataPromise = fetchPageData();
    const shell = await fetchPageShell();

    navigateEvent.commit();
    // Now the new URL is active.
    updateDOM(shell);

    shell.populate(await dataPromise);
  },
});

The commit option defaults to "immediate", which commits before calling the handler.

If commit is "manual", and navigateEvent.commit() is not called before the handler promise settles, commit is done automatically once the handler promise settles.

If commit is "manual", scroll positions are captured when navigateEvent.commit() is called. Otherwise, they're captured before calling the handler.

If commit is "manual", calling navigateEvent.scroll() before navigateEvent.commit() would… I'm not sure about this one. Since calling scroll is more of a signal to start updating scroll, maybe it should queue behind commit rather than throwing an error.

I think this pattern fits in better with how we're handling scroll and focus.

Yeah, this looks like the most promising of the proposals so far. I'll play with it and see if I can any issues.

@dvoytenko
Copy link

commit: "manual" + navigateEvent.commit() look good. I think we were discussing two-callback model to address some possible edge cases with the navigateEvent.commit(), such as:

  • In a non-manual mode, what does the navigateEvent.commit() do?
  • What if the navigateEvent.commit() do if it's called synchronously before handler callback has been called?
  • What if the handler callback successfully executes and the returned promise settles, but the commit() was never called? Will the API call it automatically?
@jakearchibald
Copy link

jakearchibald commented Jul 30, 2022

  • In a non-manual mode, what does the navigateEvent.commit() do?

It should throw, similar to navigateEvent.scroll().

  • What if the navigateEvent.commit() do if it's called synchronously before handler callback has been called?

Again, throw, similar to .scroll(). It might be neater to put .commit() on an object that's passed into the handler, as it prevents this issue, but I'm not sure it's worth splitting the API up like this.

  • What if the handler callback successfully executes and the returned promise settles, but the commit() was never called? Will the API call it automatically?

Yes, once the promise settles.

Also similar to .scroll(), .commit() will throw if it's called after the navigation has already committed (either manually or automatically).

@bathos
Copy link

bathos commented Aug 1, 2022

It might be neater to put .commit() on an object that's passed into the handler, as it prevents this issue, but I'm not sure it's worth splitting the API up like this.

Minor expression of a preference for this sort of neatness. When the/a common case for a callback API ends up requiring per-call closures for the sake of capturing effective operands / “subject objects” lexically, I see a stack of var self = this’s trying to sneak by in a trenchcoat 🥸. That’s subjective for sure — just tossing it out there as an angle to consider.

(More generally I think newcomers and oldcomers alike benefit when APIs that abstract control flow “usher” you through the flow as clearly as possible, e.g. “the intercept handler is what receives the capability to commit, so I know I can’t do it before that”.)

@jakearchibald
Copy link

@domenic want are your thoughts on this?

Should .commit go on the NavigateEvent, or on a new object passed into the handler?

navigateEvent.intercept({
  commit: "manual",
  async handler() {
    // The old URL is still active.
    const dataPromise = fetchPageData();
    const shell = await fetchPageShell();

    navigateEvent.commit();
    // Now the new URL is active.
    updateDOM(shell);

    shell.populate(await dataPromise);
  },
});

vs

navigateEvent.intercept({
  commit: "manual",
  async handler(interceptController) {
    // The old URL is still active.
    const dataPromise = fetchPageData();
    const shell = await fetchPageShell();

    interceptController.commit();
    // Now the new URL is active.
    updateDOM(shell);

    shell.populate(await dataPromise);
  },
});

The argument is, commit can only be called during the lifetime of the handler, so the scoping is neater.

The counterargument is… ugh a new object just for one method.

@domenic
Copy link
Collaborator Author

domenic commented Aug 1, 2022

Yeah, I think we have enough precedent for putting things on the NavigateEvent object, with options controlling them being passed to intercept(). I like putting it there.

@natechapin
Copy link
Collaborator

Yeah, I think we have enough precedent for putting things on the NavigateEvent object, with options controlling them being passed to intercept(). I like putting it there.

I'd lean toward following the precedent of putting it on NavigateEvent, too.

There's one extra corner case to consider with that approach, though:

navigation.onnavigate = e => {
  e.intercept({ commit: 'manual' });
  e.commit();
}

In immediate commit mode, the commit will occur when the navigate event completes. In this example, manual commit mode can be used to commit the navigation slightly before immediate commit mode would allow. I think we might want to prevent commit() during navigate event dispatch.

@natechapin
Copy link
Collaborator

One other question that came up in conversation with @domenic: how useful is { commit: 'manual' } if it is only available for non-traverse navigations?

Background:
Currently, the navigate event is cancelable only for non-traverse navigations. This is because traverals can potentially affect multiple windows simultaneously (where non-traverse navigations affect one window at a time). If one window cancels but another commits, what should we do? If we allowed them to diverge, the windows are out of sync: we are neither at the starting session history entry, nor the one we are traversing to. Our solution was to avoid that case by making the navigate event intercept()able but not cancelable.

If we introduce { commit: 'manual' } and the handler throws before commit() is invoked, that's a de facto cancel (we'll never commit). So the quickest way to get this out the door is to follow the current precedent and forbid { commit: 'manual' } for traversals, at least for now.

If deferred commit for traversals is critical, though, then I would need to go back and generally make traversal navigations cancelable, which is a pretty big implementation hurdle. It can be done, but it makes the effort:reward ratio a lot lower for this feature.

@domenic
Copy link
Collaborator Author

domenic commented Aug 2, 2022

Particularly interested in feedback from @posva, @Rich-Harris, and @atscott on @natechapin's question above!

@jakearchibald
Copy link

I think we might want to prevent commit() during navigate event dispatch.

That seems fine. From the developer's point of view, commit() cannot be called before their handler is called.

@posva
Copy link
Contributor

posva commented Aug 8, 2022

I hope I understood everything well, there was quite a lot to catch up and had to go back and read other parts of the proposal as well 😅. I tried to keep this concise:

One other question that came up in conversation with @domenic: how useful is { commit: 'manual' } if it is only available for non-traverse navigations?

IMO yes, this is still useful because in SPA routing we need a way to defer the URL update. It's far from ideal though but it's the same problem as #32. The Navigation API wants to provide a new API that allows for two models of navigations (eager and deferred) but the problem is we are still forced to use the eager model for UI based navigations. In Vue router (and I think others) we have:

  • Navigation controlled by the app (e.g. <Link /> component and router.push()): Defer the update of the URL until navigation is confirmed (e.g. resolving navigation guards, data fetching, etc) and the new content can be displayed at the same time as the URL updates (Start, Commit+Switch+Finish): deferred URL update
  • UI based navigations: Update the URL immediately while still displaying the old content, if navigation is rejected (same as above), rollback to the previous location while staying on the old content, if the navigation is resolved, switch to the new content (Start+Commit, Switch+Finish).

If deferred commit for traversals is critical, though, then I would need to go back and generally make traversal navigations cancelable, which is a pretty big implementation hurdle. It can be done, but it makes the effort:reward ratio a lot lower for this feature.

I think that for SPA routing, they are critical, pretty much like #32 because it enables both models in all scenarios.

Another possibility would make eager URL updates the new default but I don't think that's a good idea because an SPA navigation can still be canceled and that would mean the URL would change twice: once to start the navigation and again when it's canceled.

@fabiancook
Copy link

I implemented a proof of concept intercept({ commit: "manual" }) + commit() against my implementation

After using navigation with so much async, its works very clean without taking anything away.

https://github.com/virtualstate/navigation/blob/fa26599122c4e6774a0033f572480dec05f0b1e2/src/tests/commit.ts#L8-L29

@fabiancook
Copy link

fabiancook commented Sep 21, 2022

If intercept can be sync for options, and commit can then be sync... there appears no way to "report" a sync error other than throwing in the event handler, but does that just go to top level or does it cancel the transition?

Seems like it just goes uncaught:

navigation.addEventListener("navigate", event => { 
  event.intercept(); 
  throw new Error("Sync error");
})
navigation.addEventListener("navigateerror", event => { console.error({ event }) })
await navigation.navigate(`/test/${Math.random()}`).finished

image

vs

navigation.addEventListener("navigate", event => event.intercept(Promise.reject(new Error("Async error"))))
navigation.addEventListener("navigateerror", event => { console.error({ event }) })
await navigation.navigate(`/test/${Math.random()}`).finished

image

Taking the name reportError from: whatwg/html#1196

navigation.addEventListener("navigate", event => { 
  event.intercept(); 
  if (someCondition) { 
    event.reportError(new Error("Sync error")); 
  } else {
    event.commit();
  }
})
navigation.addEventListener("navigateerror", event => { console.error({ event }) })
await navigation.navigate(`/test/${Math.random()}`).finished

event.reportError would be very close to event.preventDefault, but instead of AbortError we get a user defined error.

image

self.reportError(1) -> addEventListener("error", ...)
addEventListener("navigate", event => event.reportError(1)) -> addEventListener("navigateerror", ...)

image

aarongable pushed a commit to chromium/chromium that referenced this issue Feb 27, 2023
…sync commit in the Navigation API

* Move the SawURLChange() call for a soft navigation intercept()ed by
  the NavigateEvent to NavigateEvent::DoCommit. This will eventually
  need to handle both immeidate commits and delayed async commits.
* Rename SoftNavigationHeuristics::SetBackForwardNavigationURL to
  SetAsyncSoftNavigationURL. Currently, it's only used for
  back/forward navigations because those are the only navigations that
  commit asynchronously right now. That will change when
  NavigateEvent.intercept() gets support for deferred commits
  (WICG/navigation-api#66), so give it a
  more generic name that describes the cases where it's needed.

Change-Id: I16bf7c732c7c977f928bee3cc0bf8d945aabd88b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4288932
Reviewed-by: Yoav Weiss <yoavweiss@chromium.org>
Commit-Queue: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1110422}
@domenic
Copy link
Collaborator Author

domenic commented Mar 3, 2023

Hey all!! @natechapin is working on a prototype of this in Chromium, and the explainer is available for review in #259 . You might enjoy taking a look!

@natechapin
Copy link
Collaborator

Hey everyone,

@domenic and I have been bikeshedding names in the code review protoype, and I wanted to get some additional feedback.

Based on @jakearchibald's suggestions earlier in this thread, I had used commit values of 'immediate' and 'manual'. @domenic suggested that we change 'manual' to 'after-transition'. This is for consistency with focus and scroll behavior options, and to emphasize that if navigateEvent.commit() is not called, then the commit will occur when the transition ends (in the prototype, this is just before navigatesuccess fires).

Does anyone have thoughts on the clearest name?

@bathos
Copy link

bathos commented Mar 28, 2023

+1 for 'after-transition'. Apart from communicating more than “manual”, it makes more sense in an enum alongside 'immediate' because both phrases are “temporal” choices.

@natechapin
Copy link
Collaborator

Thanks for the feedback! One more question:

If the commit is deferred, and the handler() returns a rejected Promise before navigateEvent.commit() is called, the prototype treats that as a cancelled navigation. But we want to avoid any situation in which this allows a backdoor way to cancel a navigation (e.g., navigateEvent.intercept({ commit: 'after-transition', handler: () => Promise.reject() }); to cancel an otherwise uncancellable traversal in order to back-trap a user).

Our proposal is to forbid 'after-transition' when navigateEvent.cancelable is false. The question is: what should navigateEvent.intercept({ commit: 'after-transition' }); do in that case? I see two options:

  • intercept() throws, and no intercept occurs.
  • 'after-transition' is ignored with a console warning. The navigation is still intercepted, but the commit is 'immediate' by default.

Note that this is a relatively rare situation, because it requires the navigation to be cancellable but also interceptable, which is only the case for uncancellable same-document traversals. That means you're not going to accidentally go cross-document if we choose the first option.

Thoughts?

@atscott
Copy link
Contributor

atscott commented Mar 29, 2023

'after-transition' is ignored with a console warning. The navigation is still intercepted, but the commit is 'immediate' by default.

I think this is probably more reasonable than throw, right? You might still want to intercept to get the other benefits of the transition (loading spinner, scroll restoration, etc.)

@natechapin
Copy link
Collaborator

'after-transition' is ignored with a console warning. The navigation is still intercepted, but the commit is 'immediate' by default.

I think this is probably more reasonable than throw, right? You might still want to intercept to get the other benefits of the transition (loading spinner, scroll restoration, etc.)

I think the main concern is whether unexpectedly having an 'immediate' commit when you asked for an 'after-transition' will break application logic.

@tbondwilkinson
Copy link
Contributor

I guess I have a slight preference for throwing an error, but I think it's also reasonable to intercept without throwing an error.

@atscott
Copy link
Contributor

atscott commented Mar 30, 2023

Ah, sorry I forgot that this is actually a property on the intercept options. I think it probably makes sense to throw in that case.

domenic pushed a commit that referenced this issue Apr 12, 2023
As such, call the feature "deferred commit" instead of "manual commit".

See discussion in #66 (comment).
@domenic
Copy link
Collaborator Author

domenic commented Apr 28, 2023

For anyone watching this thread, note that the explainer is updated to describe this new ability, and this new ability is available in Chromium >= 114.0.5696.0 behind the experimental web platform features flag.

This issue remains open at least until the spec is updated, which hasn't happened yet.

Since this is a tricky area, we're looking for web developer testing and validation that what we've built here is good and usable. Please play with it behind a flag and let us know if it's something we should ship!

@rd3k
Copy link

rd3k commented Jun 23, 2023

I've been using commit: 'after-transition' in a test application. So far so good! I'm finding I want to use it over 'immediate', so hope it makes the cut.

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