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

Worries about making all navigations async #19

Closed
domenic opened this issue Feb 4, 2021 · 18 comments · Fixed by #46
Closed

Worries about making all navigations async #19

domenic opened this issue Feb 4, 2021 · 18 comments · Fixed by #46
Labels
foundational Foundational open questions whose resolution may change the model dramatically

Comments

@domenic
Copy link
Collaborator

domenic commented Feb 4, 2021

The proposal currently makes all navigations asynchronous. This is to support allowing them to be intercepted by navigate and replaced by SPA navs. However, the implications of this are a bit tricky.

Note that today, at least in Firefox and Chrome,

location.hash = "foo";
console.log(location.href);

outputs the new URL, with #foo at the end. history.pushState() and history.replaceState() are a non-interoperable mess, but at least some of the time they're also synchronous. aElement.click() for a fragment navigation link is asynchronous in Firefox, synchronous in Chrome, hmm.

But the upside is that if we stick with the current proposal that location.href only updates at the "end" of the navigation, making navigations async this would be a breaking change.

There are a few potential mitigations I can think of, although none of them are great:

  • Allow location.href (and other things, like document.URL) to update, but still treat the navigation process as async, potentially reverting the URL if the navigation gets canceled. This seems very messy and likely to lead to bugs and problems, both in application code and browser code.

  • Only apply this new navigations-are-async behavior if an event listener is present for appHistory's navigate event. Adding event listeners should not have side effects, so we'd probably change the API shape to something like appHistory.setNavigateHandler() instead of using an event. This seems doable, although perhaps intrusive into browser code.

  • Only make navigations async for cases where they are already async in at least one browser, such as:

    • New APIs like appHistory.pushNewEntry()
    • Cross-document navigations (including aElement.click() for non-fragment navigations)
    • aElement.click() for fragment navigations, on the theory that other browsers can align with Firefox?
    • NOT location.hash or location.href updates that only change the hash
    • NOT history.pushState() (probably; pending further browser testing)

I'm not sure what the right path is here. It might be a more radical re-think, beyond these options. I also need to process the issues @esprehn filed around how queued-up navigations are problematic as currently proposed (#10 #11 #13); it's possible the process of solving those might change how we approach this problem.

@tbondwilkinson
Copy link
Contributor

This needs more thought for sure.

I do think that we can't change the behavior of updating the URL synchronously - that's baked in, code depends on it, and there's no way we can change it.

I think we should think about divorcing the update of a URL with the end of a navigation - there are certainly instances of where you'd want to front-load the URL update even though the page is still rendering in response to the change (and you'd want to defer other navigations while you're rendering). Doing so would allow us to continue to have navigations be asynchronous, but would allow us to continue having the URL update at the start of a location.hash navigation, e.g.

I think the worst of all worlds would be to have navigations be both synchronous and asynchronous, depending. I think everything must be either all synchronous or all asynchronous, and any middle-ground is dangerous.

@jakearchibald
Copy link

From a spec point of view, I'm already going to handle sync updates to hash & pushState in a non-legacy way, so it's fine if appHistory wants to do the same.

@domenic
Copy link
Collaborator Author

domenic commented Feb 16, 2021

Here's another possibility, which is perhaps attractive in its simplicity: only make navigations async if there's a navigate event which calls event.respondWith().

That is:

location.hash = "foo";
console.log(location.hash); // logs "foo"

appHistory.addEventListener("navigate", e => {
  e.respondWith(Promise.resolve());
});

location.hash = "bar";
console.log(location.hash); // logs "foo" still---navigation is async

Promise.resolve().then(() => {
  console.log(location.hash); // now logs "bar"
});

This... seems pretty nice? I'm curious how it fits with

I think the worst of all worlds would be to have navigations be both synchronous and asynchronous, depending. I think everything must be either all synchronous or all asynchronous, and any middle-ground is dangerous.

though.

@tbondwilkinson
Copy link
Contributor

We have to handle the case of the URL updating synchronously. What remains I think more open for debate is how and whether we support synchronous navigations.

When I approach this problem I like to think of location.hash assignments like the user literally entered in 'foo' into the URL bar and clicked enter.

What should happen when a user does that? Whatever happens when a user does that, that's probably what should happen when someone in code writes location.hash.

So I wouldn't expect to change the hash of the URL and then my URL doesn't display that change because there's some JavaScript with a e.respondWith(Promise.resolve()) callsite.

Maybe we should require that the URL is updated synchronously for any and all navigations, and that if you use e.respondWith() it delays the resolution of the navigation (and would block further navigations, unless the current one is cancelled) but the update to the URL still happens synchronously. Then, we allow people to choose a different URL once the navigation is finished, basically like a history.pushState() followed by a quick history.replaceState() when a render finishes.

@domenic
Copy link
Collaborator Author

domenic commented Feb 16, 2021

We have to handle the case of the URL updating synchronously. What remains I think more open for debate is how and whether we support synchronous navigations.

Hmm, I don't quite understand the distinction you're making there?

What should happen when a user does that? Whatever happens when a user does that, that's probably what should happen when someone in code writes location.hash.

So I wouldn't expect to change the hash of the URL and then my URL doesn't display that change because there's some JavaScript with a e.respondWith(Promise.resolve()) callsite.

Well, we have choices here, because the time taken by e.respondWith(Promise.resolve()) is not user-noticable. (It's far less than a millisecond.)

Perhaps a more salient question is, what should happen if a navigate handler has e.respondWith(new Promise(r => setTimeout(r, 10_000)))? Should location.href update now, or in 10 seconds? I guess you'd expect the URL bar to already be updated (since you just typed the new URL)... and disconnecting that from location.href is a bit scary.

Maybe we should require that the URL is updated synchronously for any and all navigations, and that if you use e.respondWith() it delays the resolution of the navigation (and would block further navigations, unless the current one is cancelled) but the update to the URL still happens synchronously. Then, we allow people to choose a different URL once the navigation is finished, basically like a history.pushState() followed by a quick history.replaceState() when a render finishes.

This sounds reasonable to me. I don't think it's the only path, but perhaps it's the most consistent, and it seems fairly promising. Let me try to write up in more detail what it would look like...

@frehner
Copy link

frehner commented Feb 16, 2021

Perhaps a more salient question is, what should happen if a navigate handler has e.respondWith(new Promise(r => setTimeout(r, 10_000)))?

as a more concrete example, I could potentially see someone wanting to make an API call during respondWith to do a permission check to see if the current user can access the new route, and then either resolve or reject accordingly.

@jakearchibald
Copy link

Could hash updates remain sync, but the respondWith is then used to update the history entry?

@domenic
Copy link
Collaborator Author

domenic commented Feb 17, 2021

Let's analyze scenarios where the promise takes a long time to settle, in this do-most-things-up-front paradigm. Here's a concrete proposal for how it would look:

Success case

appHistory.addEventListener("navigate", e => {
  e.respondWith(new Promise(r => setTimeout(r, 10_000)));
});

await appHistory.push({ url: "/foo" });
  • Synchronously:
    • navigate fires. (If it were canceled instead of respondWith()ed, we'd stop here.)
    • location.href updates
    • appHistory.current updates
      • But, it has a boolean property, appHistory.current.finished, which is false for now
    • appHistory fires currentchange
    • appHistory.current fires navigateto
    • Any now-unreachable AppHistoryEntry instances fire dispose
    • The URL bar updates, but the tab spinner is spinning (and the stop button is present? Interaction between the stop button and navigate event #38)
  • Asynchronously after 10 seconds (assuming nothing preempted the "ongoing" navigation)
    • appHistory.current.finished changes to true
    • appHistory.current fires finish
    • appHistory fires navigatefinish
    • The promise returned from appHistory.push() fulfills
    • The tab spinner stops spinning
    • Accessibility technology announces the navigation is complete?

Failure case

appHistory.addEventListener("navigate", e => {
  e.respondWith(new Promise((r, reject) => setTimeout(() => reject(new Error("bad")), 10_000)));
});

await appHistory.push({ url: "/foo" });
  • Synchronously:
    • navigate fires. (If it were canceled instead of respondWith()ed, we'd stop here.)
    • location.href updates
    • appHistory.current updates
      • But appHistory.current.finished is false for now
    • appHistory fires currentchange
    • appHistory.current fires navigateto
    • Any now-unreachable AppHistoryEntry instances fire dispose
    • The URL bar updates, but the tab spinner is spinning
  • Asynchronously after 10 seconds (assuming nothing preempted the "ongoing" navigation)
    • appHistory fires navigateerror
    • location.href and location.hash change back
    • appHistory.current changes back to the previous entry that started the navigation
    • appHistory fires currentchange
    • appHistory.current fires navigateto
    • The no-longer-current AppHistoryEntry fires dispose
    • The promise returned from appHistory.push() rejects with the given Error object
    • The tab spinner stops spinning
    • Accessibility technology announces the navigation failed??

Interruption case

This involves a bit more code to show how you are supposed to properly handle abort signals. It also uses location.hash instead of appHistory.push() because we probably want appHistory.push() during an ongoing navigation to queue. while location.hash will preempt.

appHistory.addEventListener("navigate", e => {
  e.respondWith((async () => {
    await new Promise(r => setTimeout(r, 10_000));
    
    // Since there's no setTimeout-that-takes-an-AbortSignal we check manually here.
    // A manual check isn't needed if you're doing something like fetch(..., { signal: e.signal }).
    if (!e.signal.aborted) {
      document.body.innerHTML = `navigated to ${e.destination.url}`; 
    }
  })());
});

const promise = appHistory.push({ url: "/foo" });

setTimeout(() => {
  location.hash = "bar";
}, 1_000);
  • Synchronously:
    • navigate fires. (If it were canceled instead of respondWith()ed, we'd stop here.)
    • location.href updates to /foo
    • appHistory.current updates to a new AppHistoryEntry representing /foo
      • But appHistory.current.finished is false for now
    • appHistory fires currentchange
    • appHistory.current fires navigateto
    • Any now-unreachable AppHistoryEntry instances fire dispose
    • The URL bar updates to /foo, but the tab spinner is spinning
  • Asynchronously after 1 second
    • appHistory fires navigateerror
    • location.href and location.hash change to /#bar (or should it be /foo#bar?? But the navigation to /foo didn't complete...)
    • appHistory.current changes to a new AppHistoryEntry representing /#bar
    • appHistory fires currentchange
    • appHistory.current fires navigateto
    • The no-longer-current AppHistoryEntry for /foo fires dispose
    • The event.signal for the navigation to /foo fires abort.
    • promise rejects with an "AbortError" DOMException. (Note that it does this even though the promise passed to respondWith() has not yet settled. That promise gets ignored (even if it rejects).)
    • The URL bar updates to /bar, but the tab spinner is still spinning
  • Asynchronously after 10 seconds
    • The setTimeout() promise for the /foo navigation fulfills
    • But e.signal.aborted is true so there's no updating of document.body.innerHTML.
      • Note how if the web developer did not properly deal with abort signals, this could result in a bad experience of the document displaying "navigated to /foo" despite the URL bar having moved on to /bar.
  • Asynchronously after 11 seconds
    • The setTimeout() promise for the /#bar navigation fulfills
    • Since e.signal.aborted is false, document.body.innerHTML gets set to "navigated to /#bar".
    • appHistory.current.finished changes to true
    • appHistory.current fires finish
    • appHistory fires navigatefinish
    • The tab spinner stops spinning
    • Accessibility technology announces the navigation to /#bar is complete?

Questions

  • This introduces several new pieces of API surface, namely AppHistoryEntry.prototype.finished, navigatefinish and navigateerror events on window.appHistory, and a finish event on individual AppHistoryEntrys. Do these seem reasonable? Any name bikesheds? Other names for "finish" I thought of include "settle" and "commit".

  • How does the sequencing and placement of various events sound?

  • I need more insight into the desired accessibility technology experience in these cases.

@domenic domenic added the foundational Foundational open questions whose resolution may change the model dramatically label Feb 17, 2021
@tbondwilkinson
Copy link
Contributor

This introduces several new pieces of API surface, namely AppHistoryEntry.prototype.finished, navigatefinish and navigateerror events on window.appHistory, and a finish event on individual AppHistoryEntrys. Do these seem reasonable? Any name bikesheds? Other names for "finish" I thought of include "settle" and "commit".

Bike shed: inProgress resolved transitioning

How does the sequencing and placement of various events sound?

Can you add when the navigate handler is called? It's not clear to me from these examples. I would expect it between navigateto and currentchange? Other than that, this feels right.

For the interrupt case, why does the push() abort the previous push()? Should it queue instead?

@domenic
Copy link
Collaborator Author

domenic commented Feb 18, 2021

Bike shed: inProgress resolved transitioning

What's transitioning?

Can you add when the navigate handler is called? It's not clear to me from these examples. I would expect it between navigateto and currentchange? Other than that, this feels right.

Good question...

I think it'd be weird to put it in that specific spot, because IMO we should minimize the code that runs between appHistory.current updating and appHistory firing currentchange. In fact, I think we should move navigateto after `currentchange.

Other than that, we have a lot of flexibility. It could even happen before location.href updates! I think there are a few reasonable options:

  • Before location.href updates. I.e. everything is about to update (in the same tick!), but it hasn't yet; navigate gets first crack at it.

  • After location.href updates but before updating appHistory.current and firing any other events. I.e., the navigate event is the first notification to app history; all the rest of app history is behind that one. (However this does mean window.history and appHistory will be out of sync, hmm.)

  • After all other events, including disposes. Perhaps even after the URL bar updates, although that's not observable to script.

I'm currently leaning toward the last option. WDYT?

For the interrupt case, why does the push() abort the previous push()? Should it queue instead?

Also a good question. We need to figure out our queuing plan in general.

But, what about a very similar example:

location.hash = "foo";

setTimeout(() => {
  location.hash = "bar";
}, 1_000);

My intuition was that this should not queue; it should abort. So I was thinking appHistory.push() should behave the same. We could make this queue by saying that if there's an app history navigate respondWith() promise ongoing, then location.hash changes its behavior to be asynchronous. But I think we want to avoid that?

@tbondwilkinson
Copy link
Contributor

What's transitioning?

Well, one page to another. I guess "navigating". I dunno.

Before location.href updates. I.e. everything is about to update (in the same tick!), but it hasn't yet; navigate gets first crack at it.

We kind of have to do this if we want to allow for complete URL rejections or modifications right? Otherwise the URL bar would flash until the navigate event gets sent (and then rejected).

But, what about a very similar example:

I think whether it queues or preempts depends on the operation and API. I would expect location.hash (and URL bar updates, and history.pushState) to all preempt, and no way to make them queue (unless, I suppose, you cancelled in the navigate handler and re-queued using appHistory.push). But I would expect appHistory.push() to queue by default, and preempt only if requested (can we add an extra option to appHistory.push?)

@domenic
Copy link
Collaborator Author

domenic commented Feb 18, 2021

Well, one page to another. I guess "navigating". I dunno.

Well, then I'm confused, I thought that's what inProgress is. What are the three states you see?

We kind of have to do this if we want to allow for complete URL rejections or modifications right? Otherwise the URL bar would flash until the navigate event gets sent (and then rejected).

Nah, we have 16 milliseconds of asynchronicity allowed before any URL bar flashings occur.

I think whether it queues or preempts depends on the operation and API.

Interesting, OK. That's definitely doable, and makes sense; if we don't expect complete symmetry between appHistory.push() and history.pushState() then we have more flexibility. I'll update the interruption example to use location.hash.

@tbondwilkinson
Copy link
Contributor

Well, then I'm confused, I thought that's what inProgress is. What are the three states you see?

Oh, whoops, I'm just bike shedding the name for finish. My alternatives suggestions were inProgress, resolved, transitioning, navigating. There's plenty of states let's not add a third.

Nah, we have 16 milliseconds of asynchronicity allowed before any URL bar flashings occur.

I still think that having the navigate handler go first probably makes sense - if it cancels a navigation, you wouldn't expect ANY navigateto, currentchange, etc to get sent to handlers. Receiving an event notification and then immediately after a cancel notification is probably annoying.

@domenic
Copy link
Collaborator Author

domenic commented Feb 18, 2021

Ah, understood about the names, and agreed about going first. I'll update the above description to reflect that.

Now, next up is to do the same analysis for cross-document navigations, where location.href normally doesn't change instantly...

@domenic
Copy link
Collaborator Author

domenic commented Feb 24, 2021

I started writing up a counterpart analysis to #19 (comment) for normally-cross-document navigations, like location.href = "/foo". The difference here, compared to the case in the original post, is that today

location.href = "/foo";
console.log(location.href);

will log the old location. (And then the JS will quickly stop running as the navigation proceeds to unload the current document.)

I quickly found that an in-depth analysis isn't really needed. We basically have two choices:

  1. Treat them like the intercepted same-document navigations in Worries about making all navigations async #19 (comment). So, for example, if you do location.href = "/foo" and then intercept it with a navigate handler, we immediately update location.href, update appHistory.current, etc. even though this wouldn't happen normally.

  2. Move all of the interesting stuff to later. So e.g. don't update location.href or appHistory.current until the promise passed to respondWith() settles.

(1) feels much better to me. The intent of navigate interception is to convert cross-document navigations into same-document ones, and that fits better with that spirit.

The only advantage of (2) is that it preserves the property where location.href = "/foo"; console.log(location.href); logs the old location. But I think that's fine; if you're intentionally converting cross-document navs into same-document navs, you should expect such a change.

I'll start working on an explainer pull request that incorporates this.

domenic added a commit that referenced this issue Feb 25, 2021
This includes ones generated by intercepting the navigate event with event.respondWith(). However, the promise passed to event.respondWith() is still useful for signaling the navigation's successful or unsuccessful completion. That is used to fuel browser UI, such as the loading spinner, as well as promises and events. And a rejected promise passed to event.respondWith() will roll back the navigation.

Closes #19. Closes #44.
@domenic
Copy link
Collaborator Author

domenic commented Feb 25, 2021

I've posted a draft at #46.

Stepping back, I think we have three options:

  1. All same-document navigations (including cross-document-converted-to-same-document with event.respondWith()) are sync, i.e. location.href, history.state, appHistory.current, URL bar, etc. update synchronously. Extra things happen when the promise passed to event.respondWith() settles. That's what we've been discussing recently and that I detailed in Make all same-document navigations sync #46.

  2. All non-intercepted same-document navigations are sync. All intercepted same-document navigations are async, i.e. location.href, history.state, appHistory.current, URL bar, etc. update only after the promise passed to event.respondWith() settles. (This relies on the fact that doing appHistory.addEventListener("navigate", e => e.respondWith(...)) is a kind of opt-in to the new async behavior, so compat problems are avoided.)

  3. A hybrid with more direct developer control. Here, all non-intercepted same-document navigations are sync. All intercepted same-document navigations have two potentially-async lifecycle points: "navigation commit", where location.href, history.state, appHistory.current, URL bar, etc. update. And "navigation finish", where extra things happen (like in (1)).

An idea of what (3) could look like would be

appHistory.addEventListener("navigate", e => {
  // "Navigation finish" happens when this promise settles
  e.respondWith((async () => {
    const res = await fetch(e.destination.url);
    
    // OK, we got this far; we're ready to commit:
    e.commitNavigation();
    
    // Now do more stuff in between "navigation commit" and "navigation finish"
    const template = await res.text();
    const html = await myCoolAsyncTemplateEngine(template, e.destination.state);
    document.body.innerHTML = html;
  })());
});

Overall I'm reasonably happy with (1), but I wanted to make sure we're considering all the options.

@tbondwilkinson
Copy link
Contributor

I think I'm also happy with 1. 3 is kinda appealing just from a control perspective, but I'm not convinced it's a good tradeoff for complexity.

domenic added a commit that referenced this issue Feb 25, 2021
This includes ones generated by intercepting the navigate event with event.respondWith(). However, the promise passed to event.respondWith() is still useful for signaling the navigation's successful or unsuccessful completion. That is used to fuel browser UI, such as the loading spinner, as well as promises and events. And a rejected promise passed to event.respondWith() will roll back the navigation.

Closes #19. Closes #44. Still some discussion to be had on the failure case in #47.
@Yay295
Copy link

Yay295 commented Feb 26, 2021

For "2. All intercepted same-document navigations are async", I'd think that could cause issues if someone is updating pieces of their code at a time. If some of their code relies of navigations being synchronous, and then they start using this new API (or they include a library that uses this new API), it could break their code that hasn't been updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
foundational Foundational open questions whose resolution may change the model dramatically
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants