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

Add appHistoryEntry.id and clarify object identity #88

Merged
merged 3 commits into from
Apr 2, 2021
Merged

Add appHistoryEntry.id and clarify object identity #88

merged 3 commits into from
Apr 2, 2021

Conversation

domenic
Copy link
Collaborator

@domenic domenic commented Mar 30, 2021

Closes #7.

Would love a look from @tbondwilkinson in addition to @natechapin. With this in place I'll feel more comfortable speccing a basic AppHistoryEntry and appHistory.current.

@domenic domenic requested a review from natechapin March 30, 2021 22:41
@frehner
Copy link

frehner commented Mar 30, 2021

The section explaining the differences between key and id is great. 💯

One thing that I just thought of, that probably isn't that important but I was curious about: does the AppHistoryEntry object reference stay the same in all instances? e.g.

const previous = appHistory.current
await appHistory.navigate({replace: true, state: ''})
const current = appHistory.current

console.log(previous.id === current.id) // false
console.log(previous.key === current.key) // true

console.log(previous === current) // ???

@domenic
Copy link
Collaborator Author

domenic commented Mar 30, 2021

The plan is for the identity to match id. (Except in cases where that's impossible and doesn't make sense, like browser session restore.Well, probably we'll generate a new id even for those, now that I think about it. So it'll match 100% of the time.)

The explainer tries to talk about this in "The complete list of ways the current app history entry can change to a new entry are:" but maybe it'd be best to be more explicit...

@domenic
Copy link
Collaborator Author

domenic commented Mar 30, 2021

TODO: talk about session restore in more detail. I think it should be fine to generate new IDs upon session restore (while keys stay the same).

@tbondwilkinson
Copy link
Contributor

Overall looks great.

I think I would be a little surprised if IDs are regenerated upon session restore, because there are lots of caches you might have stored history-correlated resources that are preserved across a session restore

@domenic
Copy link
Collaborator Author

domenic commented Mar 31, 2021

Hmm OK, that makes sense. We can probably make them stick around across session restore.

@domenic domenic merged commit d13d48b into main Apr 2, 2021
@domenic domenic deleted the add-id branch April 2, 2021 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update vs. replace, entry mutability, and keys
3 participants