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

event.destination should not be an AppHistoryEntry. How should it behave? #97

Closed
domenic opened this issue Apr 16, 2021 · 6 comments
Closed
Labels
addition A proposed addition which could be added later without impacting the rest of the API feedback wanted

Comments

@domenic
Copy link
Collaborator

domenic commented Apr 16, 2021

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 with appHistory.entries() or appHistory.current after the navigation. And, adding event listeners doesn't make much sense. And, the semantics of index were strange.

So we should definitely divorce event.destination from the AppHistoryEntry instances that live in appHistory.entries(), and instead make it a more abstraction representation of the destination. I think reusing much of the AppHistoryEntry properties makes sense, but I have some questions for each of them:

  • url: OK, well, this is still obvious
  • getState(): this seems fine and useful to include
  • key: For push navigations, should key be the future key, or should it maybe be null? 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...
    • Don't include this
    • -1 for push navigations, accurate index for other navigations
    • appHistory.current.index + 1 for push navigations, accurate index for other navigations
  • sameDocument: If you call event.respondWith(), should this change, or should it always reflect the "original" navigation? (AppHistoryEntry.sameDocument #70)
@tbondwilkinson
Copy link
Contributor

Thoughts:

  1. key: If possible, I think it would be useful. But if not, it's not a big deal.
  2. id: If possible, I think it would be useful. But if not, it's not a big deal.
  3. index: I lean towards not including. I think in general we want to discourage positional-type behavior. If this is a directional navigation, I think you could figure this out using key/id if you really wanted it by looking in appHistory.entries()
  4. Tricky! I think it is probably simpler to think of the event Object as a static, immutable thing, rather than as a mutable reflection of state. In that sense, I lean towards not updating it.

@domenic
Copy link
Collaborator Author

domenic commented Apr 16, 2021

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.

@tbondwilkinson
Copy link
Contributor

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.

@dvoytenko
Copy link

I think it should be whatever appHistory.navigate() accepts. I'd think of it as a not-yet-materialized history entry, so I don't think it'd have an id yet or a key (unless the key has been provided in the navigate() call).

@domenic domenic added the addition A proposed addition which could be added later without impacting the rest of the API label May 5, 2021
@domenic
Copy link
Collaborator Author

domenic commented May 27, 2021

I started working on a PR to add key to "traverse" navigations. However then I thought that for traverse navigations, key/id/index are all on equal footing, and probably we should include all of them if we're going to include any of them. Hmm.

Maybe exclude index because it's really about the entry that lives in appHistory.entries(), whereas we're trying to say that e.destination is not the entry?

Or maybe e.destination should be either an AppHistoryEntry (for traverse) or an AppHistoryDestination (for push/replace/reload)? Reload being on that side feels a bit weird but it seems necessary since appHistory.reload({ state: newState }) means e.destination.getState() needs to be different from appHistory.current.getState()...

domenic added a commit that referenced this issue Jul 19, 2021
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.
domenic added a commit that referenced this issue Jul 23, 2021
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.
domenic added a commit that referenced this issue Jul 23, 2021
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.
@domenic
Copy link
Collaborator Author

domenic commented Jul 28, 2021

I split the remaining discussions into #138 and #139. In both cases I am pretty happy with the status quo but they are just tracking issues to make sure we can gather feedback if people disagree.

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

No branches or pull requests

3 participants