-
Notifications
You must be signed in to change notification settings - Fork 26
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
event.destination should not be an AppHistoryEntry. How should it behave? #97
Comments
Thoughts:
|
I'd love to hear more cases for using key and id (for pushes) in a navigate handler. I dislike the special case of making them null for pushes, but having real use cases would help me justify fixing that. Otherwise, the path of least resistance for spec/implementation is to probably leave them null. |
I've seen a few use cases of storing the key so that you can detect when the state changes away and take some action. But perhaps they could set up those listeners directly on the entry? If we're reasonable sure no one wants to correlate an in-progress entry with a serialized-entry, then I think they might not be necessary. |
I think it should be whatever |
I started working on a PR to add Maybe exclude Or maybe |
This adds key, id, and index to "traverse" navigations. For non-traverse navigations they are currently null, null, and -1 respectively. This takes care of most of #97 but there's still some potential discussion about non-traverse navigations and about AppHistoryDestination vs. AppHistoryEntry. Also fixes getState() on AppHistoryDestination to return undefined instead of null, to match getState() on AppHistoryEntry. Editorially this involves a refactoring to construct the AppHistoryDestination object outside of the inner navigate event firing algorithm, and pass it in.
This adds key, id, and index to "traverse" navigations. For non-traverse navigations they are currently null, null, and -1 respectively. This takes care of most of #97 but there's still some potential discussion about non-traverse navigations and about AppHistoryDestination vs. AppHistoryEntry. Also fixes getState() on AppHistoryDestination to return undefined instead of null, to match getState() on AppHistoryEntry. Editorially this involves a refactoring to construct the AppHistoryDestination object outside of the inner navigate event firing algorithm, and pass it in.
This adds key, id, and index to "traverse" navigations. For non-traverse navigations they are currently null, null, and -1 respectively. This takes care of most of #97 but there's still some potential discussion about non-traverse navigations and about AppHistoryDestination vs. AppHistoryEntry. Also fixes getState() on AppHistoryDestination to return undefined instead of null, to match getState() on AppHistoryEntry. Editorially this involves a refactoring to construct the AppHistoryDestination object outside of the inner navigate event firing algorithm, and pass it in.
As I started trying to spec event.destination, I realized making it an
AppHistoryEntry
would not work very well. In particular, only sometimes would we be able to get the right object identity, i.e. the one that matches up withappHistory.entries()
orappHistory.current
after the navigation. And, adding event listeners doesn't make much sense. And, the semantics ofindex
were strange.So we should definitely divorce
event.destination
from theAppHistoryEntry
instances that live inappHistory.entries()
, and instead make it a more abstraction representation of the destination. I think reusing much of theAppHistoryEntry
properties makes sense, but I have some questions for each of them:url
: OK, well, this is still obviousgetState()
: this seems fine and useful to includekey
: For push navigations, shouldkey
be the future key, or should it maybe benull
? This is clearly a must for traverse navigations, and is easy to compute for replace navigations.id
: should this even be included? Is it useful to make the browser figure this out early?index
: a few options...-1
for push navigations, accurate index for other navigationsappHistory.current.index + 1
for push navigations, accurate index for other navigationssameDocument
: If you callevent.respondWith()
, should this change, or should it always reflect the "original" navigation? (AppHistoryEntry.sameDocument #70)The text was updated successfully, but these errors were encountered: