Skip to content
This repository was archived by the owner on Aug 21, 2026. It is now read-only.

Make all same-document navigations sync - #46

Merged
domenic merged 3 commits into
mainfrom
sync-navs
Feb 25, 2021
Merged

domenic merged 3 commits into
mainfrom
sync-navs

Conversation

@domenic

@domenic domenic commented Feb 25, 2021

Copy link
Copy Markdown
Collaborator

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.

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.

@frehner frehner left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the introduction of appHistoryEvent.signal 👍

Comment thread interception-details.md
1. `appHistory.current` updates. `appHistory.current.finished` is `false`.
1. `currentchange` fires on `window.appHistory`.
1. `appHistory.current` fires `navigateto`.
1. Any now-unreachable `AppHistoryEntry` instances fire `dispose`.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for delayed failures, what happens to these "now-unreachable" Entries? should they become "reachable" again since the navigation was cancelled and appHistory.current was rolled back to its previous value?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great question.

I'm inclined to say that they're gone forever? Splicing a future history back into existence seems pretty weird. But in general the introduction of fallible single-page navigations raises a lot of questions, hmm...

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll open a dedicated issue about this and then point to it from here.

Comment thread interception-details.md
1. The URL bar updates to show `/foo`.
1. Any loading spinner UI starts.

After the promise rejects in ten seconds:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should there be an additional entry in the list below about the appHistoryEvent.signal being aborted?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. The signal property is specifically for aborts. I.e., it's meant to indicate that something else aborted the navigation, not that the navigate handler completed the navigation with a failure.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

appHistory.update async or not, and the effect on navigation cancellation Worries about making all navigations async

2 participants