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

How to detect or await a pending navigation if you didn't initiate it? #11

Closed
esprehn opened this issue Feb 3, 2021 · 10 comments · Fixed by #90
Closed

How to detect or await a pending navigation if you didn't initiate it? #11

esprehn opened this issue Feb 3, 2021 · 10 comments · Fixed by #90
Labels
addition A proposed addition which could be added later without impacting the rest of the API

Comments

@esprehn
Copy link

esprehn commented Feb 3, 2021

ex. Gmail uses a global queue, if there's a pending navigation it won't process any future requests to navigate until that's done. The opposite of the photo gallery example. If you click Next Page 20 times, nothing happens until the next page has loaded.

@tbondwilkinson
Copy link
Contributor

I do think that this type of coordination code should be done by the application itself- which assumedly has a navigation handler itself, and has probably some type of router that's installed via that route.

So, the router knows about all pending navigations, and if some part of the application needs to also know, it should ask the router.

Figuring out how this works on pages where some piece of logic CANNOT talk to the central router feels out of scope, and is a case we've punted on in other categories as well.

@esprehn
Copy link
Author

esprehn commented Feb 11, 2021

@tbondwilkinson I don't think it's reasonable to require a central router abstraction over top of the platform in this case. That's a major issue with the existing API surface of pushState() where ex. code that doesn't use react-router will break the app.

I don't think it's reasonable to say that no part of the page can call pushNewEntry() directly, and instead app components must go through some router library. That's also antithetical to the concept of web components where you can have two unrelated frameworks on the page like React and Lit and your app will still work.

@tbondwilkinson
Copy link
Contributor

Sorry, let me be more clear.

I think that anyone can call pushNewEntry() directly, but there needs to be some central logic that handles the conducting of navigations (deciding whether to cancel, etc.) and that would be the "router" that's installed via the navigate event listener. I don't think you'd want to load some third-party code on your page that could then reject navigations from other parts of the page, and especially the "hosting" page.

I do agree there's still some work to be done to enable third-party code to work with history, which someone brought up in #18. I don't think that any app component needs to be aware of what the centralized router implementation is, and should be able to use the native framework itself to get all information and make navigation requests as necessary. But I also don't think that any part of the app component needs to be listening directly to ALL navigations when they only really care about a portion of the URL or a portion of the history state.

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

domenic commented Mar 11, 2021

#68 attempts to address many of the surrounding issues around queued-up navigations, by getting rid of them: new navigations interrupt ongoing ones.

However, the essential problem this issue gets at in its OP still remains, which is: what if I want to wait on an ongoing navigation? How do I do that?

You can do so in the post-#68 explainer by listening for the navigatefinish + navigateerror events, and only doing work after those. However, this is not very ergonomic. I think a promise would be much nicer: something like

appHistory.nextOrOngoingNavigation.then(theNavigationSucceeded, theNavigationFailed)

where nextOrOngoingNavigation gets replaced whenever it settles.

What do folks think? Suggestions for a better name are very appreciated as well.

@frehner
Copy link

frehner commented Mar 11, 2021

More for curiosity's sake: is there a reason that navigatefinish and navigateerror must be different? or could the event that fires on navigatefinish have an error property that is only there if there was an error? that way there's only one listener for knowing when navigation is finished.

that said, I also do like the ergonomics of the promise.

@domenic
Copy link
Collaborator

domenic commented Mar 11, 2021

They don't have to be different, but it fits with other web platform patterns: e.g. onload and onerror pairs all over the place.

@tbondwilkinson
Copy link
Contributor

The one wrinkle in this is that I want to make sure this works:

while (appHistory.nextOrOngoingNavigation) {
  await appHistory.nextOrOngoingNavigation;
}
appHistory.push(url)

That is, someone else might beat you to initiating a navigation, so if that's the case, the Promise should synchronously exist.

If we don't want the nextOrOngoingNavigation field to be undefined when there's not a navigation, at least we should have a field that identifies when one exists?

@domenic
Copy link
Collaborator

domenic commented Mar 12, 2021

Very interesting example. This would be the first nullable promise on the web platform if we went that route. (In fact Web IDL does not currently allow nullable promises, but I suspect that was based on thinking of them mostly as inputs...) I kind of like the idea of making it nullable, though... then its name could just be ongoingNavigation.

However, if we kept the non-nullable "next-or-ongoing" semantics, then you could do

while (!appHistory.current.finished) {
  await appHistory.nextOrOngoingNavigation;
}
appHistory.push(url);

Hmm, maybe that suggests a different bikeshed color?

while (!appHistory.current.finished) {
  await appHistory.current.whenFinished;
}
appHistory.push(url);

This latter is kind of nice because then we don't have the replacing-itself promise; each entry just gets its own promise, which once settled, never changes.

(Although, it's a bit strange for both finished and whenFinished to live on non-current entries... maybe they should both get moved up to appHistory? Or maybe appHistory.current should be a special subclass of AppHistoryEntry?)

@frehner
Copy link

frehner commented Mar 12, 2021

or this is another option, but doesn't have that promise-based interaction you're looking for.

if(!appHistory.current.finished) {
  appHistory.current.onfinish = () => {
    appHistory.push(url)
  })
}

@domenic domenic added addition A proposed addition which could be added later without impacting the rest of the API and removed foundational Foundational open questions whose resolution may change the model dramatically labels Mar 16, 2021
domenic added a commit that referenced this issue Apr 2, 2021
This gives insight into any "ongoing" navigation, i.e. a navigation that hasn't yet reached navigationsuccess/navigationerror because the promise passed to respondWith() has not yet settled.

Additionally:

* Removes the finished property from every AppHistoryEntry, which is nice because it only ever really made sense on the current entry; it was about the transition. Instead, the presence or nullness of appHistory.transition can be used.
* Adds a type property to AppHistoryNavigateEvent since we're going to have it on appHistory.transition anyway so having it earlier (before respondWith() is called) seems very reasonable.

Closes #86. Closes #11 by adding the appHistory.transition.finished promise.

Helps with #41 as you can retrieve data from the replaced entry, at least during the transition, with appHistory.transition.previous.

Probably helps with #14 (although currentchange needs a bit of an overhaul) since appHistory.transition has the sort of useful information discussed there, such as the previous entry or the type of navigation.
domenic added a commit that referenced this issue Apr 5, 2021
This gives insight into any "ongoing" navigation, i.e. a navigation that hasn't yet reached navigationsuccess/navigationerror because the promise passed to respondWith() has not yet settled.

Additionally:

* Removes the finished property from every AppHistoryEntry, which is nice because it only ever really made sense on the current entry; it was about the transition. Instead, the presence or nullness of appHistory.transition can be used.

* Adds a type property to AppHistoryNavigateEvent since we're going to have it on appHistory.transition anyway so having it earlier (before respondWith() is called) seems very reasonable.

Closes #86. Closes #11 by adding the appHistory.transition.finished promise.

Helps with #41 as you can retrieve data from the replaced entry, at least during the transition, with appHistory.transition.from.

Probably helps with #14 (although currentchange needs a bit of an overhaul) since appHistory.transition has the sort of useful information discussed there, such as the previous entry or the type of navigation.
@domenic
Copy link
Collaborator

domenic commented Aug 10, 2021

For posterity, the way to do #11 (comment) in the current explainer is

while (appHistory.transition) {
  await appHistory.transition.finished;
}
appHistory.navigate(url);

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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants