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

appHistory.update async or not, and the effect on navigation cancellation #44

Closed
frehner opened this issue Feb 21, 2021 · 1 comment · Fixed by #46
Closed

appHistory.update async or not, and the effect on navigation cancellation #44

frehner opened this issue Feb 21, 2021 · 1 comment · Fixed by #46

Comments

@frehner
Copy link

frehner commented Feb 21, 2021

In the example section there is

// Updates the URL shown in the address bar, as well as the url property. `state` stays the same.
appHistory.update({ url });

// You can also explicitly null out the state, instead of carrying it over:
appHistory.update({ url, state: null });

// Only updates the state property.
appHistory.update({ state });

// Update both at once.
appHistory.update({ url, state });

which seems to indicate that it's not async. There is also the WebIDL format:

undefined update(optional AppHistoryEntryOptions options = {});
undefined update(AppHistoryNavigationCallback);

However, there is one place where it is written with await

use await appHistory.update({ state, url })

But the real reason for bringing this up is to understand better how appHistory.update and the navigate event are intended to work together:

If update is sync, then does that mean event.respondWith() has no effect and you cannot cancel an update with it? Would navigate event handlers need to differentiate between push and update in those cases? Or would it still work and just roll back the update at some future point, possibly firing the navigate event again?

Also, if update and push are different, does that open the door to potential mixups by developers mis-remembering which needs to be await-ed or not? (specifically regarding this section?)

Hopefully my questions make sense, and also I hope I didn't miss a section in the docs that explain this. 😃 Thanks!

(edit: This probably ties somewhat into the conversation happening in #19 as well)

@domenic
Copy link
Collaborator

domenic commented Feb 22, 2021

Heya, great question. I think I've flipped back and forth on this a few times, which is what led to the current inconsistent state you noticed.

In general, the spec's current idea of making all navigations async so that they can be intercepted doesn't really work. We're figuring that out in #19, as you saw.

The latest thinking there is that, generally, we'll update the URL/state/current entry immediately, let the potentially-async navigate handler(s) run, and when those finish, fire some extra events, fulfill promises, etc. This gets more complicated for cases like the promise passed to respondWith() rejecting, or a navigation interrupting an existing ongoing navigation. See #19 (comment) for the full current plan.

I think, given that, we should probably make both update() and push() async, with the same type of semantics.

Although, see also the discussion #42 ...

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 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.
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

Successfully merging a pull request may close this issue.

2 participants