-
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
Update vs. replace, entry mutability, and keys #7
Comments
I think we need a stable key or index to handle the react View key. For example if you navigate to /inbox and then /message we'll generate a react tree like: <App>
<Stack>
<Inbox appHistoryEntry={inboxEntry} state={inboxEntry.state} key={inboxEntry.key} />
<Message appHistoryEntry={messageEntry} state={inboxEntry.state} key={inboxEntry.key} />
</Stack>
</App> If pushing a new state changes the key it would destroy that view. Should we be using the index in the entries() list as the key instead? react-router creates a new object for each replace() even if the url is actually the same and the only thing that changed is the state. That's required us to manually memoize the url state to avoid huge re-renders of the entire page. Ideally the thing that contains the url would have stable object identity so you know you're on the same "view" and things that only care about the url don't update, even if the state object is changing. |
That concrete example is super-helpful; thank you! So it sounds like most of your cases would be served by the current proposal, i.e. update instead of replace, with
It seems like there's three potential keys here: the object identity, a |
I think entry mutability would simplify a lot of common actions. The main downside I see is that it could lead to people writing code that mutates the history state all over the place. On the other hand you can do this anyways by replacing the state; it's just more tedious. |
@domenic maybe I misunderstood the original post here, it seemed to suggest both removing object identity for Entries as well as changing the Something like:
where entry's identity changes on all mutations, but the top level object and key don't change. I don't have a good name for that though. |
@esprehn I think this is where a key that persists on replaces comes in handy. {
id: string; // This is a unique identifier for a given entry, on replace, it changes.
key: string; // This is a unique identifier for a given entry SLOT, on replace, it stays the same.
url: string;
state: any;
} Then in React:
I'm not super familiar with React, but I suspect this works? I think it's undesirable to allow mutable history entries, because it implies things like this are synchronous: appHistory.currentEntry.state.foo = 'bar';
console.log(appHistory.currentEntry.state.foo); // should it be bar?
appHistory.currentEntry.url = '#bar';
console.log(appHisotry.currentEntry.url) // is it #bar? This gets really confusing. If we allow SOME changes to Entries to be mutable, but other to be immutable because they trigger navigations (like changing the URL), then I think our API becomes pretty surprising. My opinion would be that immutability is desirable, and replace() operations more closely align with the existing history API and would make interop and migrations from it easier. |
@tbondwilkinson I don't understand what you're saying there, the state object is mutable already today. history.pushState({ test: 2 }, null, '');
console.log(history.state.test);
history.state.test = 3;
console.log(history.state.test); and it looks mutable in the current spec too? Are you saying you want the platform to deeply freeze the state object? I'm not suggesting the url field on Entry is writable. |
The state Object is "mutable" - this is actually a sore spot in the current API, I think.
Mutations to history.state are allowed but they don't actually change the serialized value that the browser uses. This is pretty surprising - only a replaceState or pushState would change the serialized value, even though the browser allows you to "modify" the state Object directly. |
So to be clear, I would like to fix this in the new API to make it so that you cannot even modify the appHistory's currentEntry state. |
Hmm, that's an interesting proposal. It would definitely require more structure then; we couldn't use an arbitrary JavaScript value (since those are always mutable). I tend to agree that's a sharp edge of the current API though, so I think fixing it is a good direction. |
To respond to
I think a lot of options are/were on the table, and some of them are orthogonal. I apologize for the over-long OP. Let me try a shorter proposal and get your impression. I'm relatively convinced we need separate
Do folks have thoughts between these two? (Note that I think either model could work with some immutable form of state, if we went that route.) |
If you replace the state I wouldn't really think of it as just the object identity being replaced, but the entire object. That is, the original object could still exist somewhere (at least until it's garbage-collected) and you're now dealing with a new object with its own identity. In this case you wouldn't really need the I don't really see the benefit of invalidating a key. If you want a new history entry you can just create one.
As for state immutability, I think it's only the current entry that causes issues. Changing the values of a non-active entry shouldn't affect anything. Being able to change the current state and then not have it actually be saved is an issue, but I don't think that's the fault of it being mutable. We just have to say that changes to a history state need to be persisted. |
I moved the discussion on state immutability to #36. |
This is my preference because I don't want someone to think that they can use |
If you do Certainly |
Also, we never really answered: should |
I do think it should change the I think making a distinction between state and URL identities is potentially confusing, because I think "state" is just non-user-visible identifying state. If you want to link metadata about history that does not constitute the identity of an entry, you can put that elsewhere pretty easily |
If the ID changes when you update the state, what happens to the old ID? Does it just become invalid, or does the old state stick around, or does the old ID refer to the new state? What is the purpose of the ID supposed to be? I think the object identity not changing could be difficult implementation wise, though I don't know exactly how browsers determine an objects identity. |
I think the id is meant to provide a unique identifier to stash resources about that "entry". So, if you update the state, that's no longer the same entry, because maybe something about that state was influencing what UI you're displaying (like scroll positions, or images that you're caching), so you may want to drop them. |
(Moved from slightlyoff/history_api#21 by @StringEpsilon, summarized by me.)
Update vs. replace
The current proposal has
appHistory.updateCurrentEntry({ state, url })
. Although it doesn't state so explicitly, the implication of this method name is that the sameAppHistoryEntry
will have itsstate
andurl
values updated. That is, it makesAppHistoryEntry
s mutable, at least when they are the current entry. Notably, thekey
value would stay the same. Call this the "mutable history entry model".An alternative approach would be to have
appHistory.replaceCurrentEntry({ state, url })
. This would generate a newAppHistoryEntry
with the providedstate
andurl
. It would probably have a newkey
(but see below). Call this the "immutable history entry model".Also, note that the mutable model could have
appHistory.replaceCurrentEntry()
as well asappHistory.updateCurrentEntry()
. Probably that would just be confusing, but it makes conceptual sense.(For the record:
window.history
takes a hybrid approach. The currentwindow.history
history entry's state is mutable (viahistory.state
), but if you want to change the page's current URL, you need to replace the current history entry usinghistory.replaceState()
.)In general in programming, immutability can make things easier with less to worry about. That might be the case here, so I'm leaning toward switching to an immutable history entry model.
The meaning and uses of
key
In @tbondwilkinson's work on creating nice application-level wrappers over
window.history
, they have (at least) two keys:His apps use the slot key to navigate around, using the equivalent of
appHistory.navigateTo(slotKey)
. Whereas the unique identifier is used to cache resources that are too big to store instate
, or to otherwise correlate a particular history entry with some out of band resource.The app history proposal, with
updateCurrentEntry()
, currently has only the "slot key". If we changed to the immutable history entry model, withreplaceCurrentEntry()
, we could either:key
as the slot key (so, the newAppHistoryEntry
would have the samekey
value as the old one), orkey
to be a unique identifier (so, the newAppHistoryEntry
would generate a newkey
UUID).We could also introduce both types of keys, or switch
key
to be a unique identifier and introduceindex
, or similar.A potential issue with unique keys is that we need to define the behavior for what happens if you do something like
i.e. if you try to navigate to a key which is no longer in the current history entries list. I think we'd just fail, probably in the same way
appHistory.back()
fails if you're at the beginning of the history list, orappHistory.navigateTo("non-existant-key")
fails.Replacing and the old entry
If we did switch to an immutable history model with
appHistory.replaceCurrentEntry()
, we could consider adding a feature likeappHistory.currentEntry.replacedEntry
pointing to the now-replaced entry. This would extend indefinitely (?), so you could doappHistory.currentEntry.replacedEntry.replacedEntry
etc.This also helps with the potential data loss, where calling
replaceCurrentEntry()
, orupdateCurrentEntry()
, will destroy anystate
value you had there. On the other hand, maybe this sort of "data loss" is intentional, in that it's better not to use up memory and storage space on replaced entries. Hmm.The text was updated successfully, but these errors were encountered: