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

Make all same-document navigations sync #46

Merged
merged 3 commits into from
Feb 25, 2021
Merged

Make all same-document navigations sync #46

merged 3 commits into from
Feb 25, 2021

Conversation

domenic
Copy link
Collaborator

@domenic domenic commented 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.

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

@frehner frehner left a comment

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 👍

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

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

1. The URL bar updates to show `/foo`.
1. Any loading spinner UI starts.

After the promise rejects in ten seconds:
Copy link

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
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 join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants