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

Update vs. replace, entry mutability, and keys #7

Closed
domenic opened this issue Feb 3, 2021 · 18 comments · Fixed by #88
Closed

Update vs. replace, entry mutability, and keys #7

domenic opened this issue Feb 3, 2021 · 18 comments · Fixed by #88
Labels
change A proposed change to the API, not foundational, but deeper than the surface

Comments

@domenic
Copy link
Collaborator

domenic commented Feb 3, 2021

(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 same AppHistoryEntry will have its state and url values updated. That is, it makes AppHistoryEntrys mutable, at least when they are the current entry. Notably, the key 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 new AppHistoryEntry with the provided state and url. It would probably have a new key (but see below). Call this the "immutable history entry model".

Also, note that the mutable model could have appHistory.replaceCurrentEntry() as well as appHistory.updateCurrentEntry(). Probably that would just be confusing, but it makes conceptual sense.

(For the record: window.history takes a hybrid approach. The current window.history history entry's state is mutable (via history.state), but if you want to change the page's current URL, you need to replace the current history entry using history.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:

  • A "unique identifier" for the history entry, which is changed whenever the entry is replaced (i.e. whenever the URL of the entry changes). (Does it change if the state changes??)
  • A "slot key" for the history entry, which stays stable even when the entry is replaced.

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 in state, 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, with replaceCurrentEntry(), we could either:

  • keep key as the slot key (so, the new AppHistoryEntry would have the same key value as the old one), or
  • switch key to be a unique identifier (so, the new AppHistoryEntry would generate a new key UUID).

We could also introduce both types of keys, or switch key to be a unique identifier and introduce index, or similar.

A potential issue with unique keys is that we need to define the behavior for what happens if you do something like

const uniqueKey = appHistory.currentEntry.key;
appHistory.replaceCurrentEntry();
appHistory.navigateTo(uniqueKey);

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, or appHistory.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 like appHistory.currentEntry.replacedEntry pointing to the now-replaced entry. This would extend indefinitely (?), so you could do appHistory.currentEntry.replacedEntry.replacedEntry etc.

This also helps with the potential data loss, where calling replaceCurrentEntry(), or updateCurrentEntry(), will destroy any state 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.

@esprehn
Copy link

esprehn commented Feb 3, 2021

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.

@domenic
Copy link
Collaborator Author

domenic commented Feb 3, 2021

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 key staying the same? Do you see a use for a key that changes if you change the state or URL? It sounds like the immutability of react-router is mostly not-helpful for you; is that right, or does it have any positives?

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.

It seems like there's three potential keys here: the object identity, a key property, and the url property. I'm pretty sold on url not being unique enough for this purpose, but can you expand on why you think the object identity is more useful than a key property?

@Yay295
Copy link

Yay295 commented Feb 4, 2021

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.

@esprehn
Copy link

esprehn commented Feb 9, 2021

@domenic maybe I misunderstood the original post here, it seemed to suggest both removing object identity for Entries as well as changing the key on each mutation. The major issue we have with react-router is it seems to create a new object one very render even if the internal values are the same. Ideally we'd have a stable string key for the route entry itself, and the underlying data would be immutable and swap on mutation.

Something like:

{
  key: string; // uuid
  entry: {
    url: string;
    state: any;
  }
}

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.

@tbondwilkinson
Copy link
Contributor

@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:

<App>
  <Stack>
    <Inbox appHistoryEntry={inboxEntry} state={inboxEntry.state} key={inboxEntry.key} />
    <Message appHistoryEntry={messageEntry} state={inboxEntry.state} key={inboxEntry.key} />
  </Stack>
</App>

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.

@esprehn
Copy link

esprehn commented Feb 11, 2021

@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.

@tbondwilkinson
Copy link
Contributor

The state Object is "mutable" - this is actually a sore spot in the current API, I think.

history.pushState({ test: 2 }, null, '');
console.log(history.state.test); // 2
history.state.test = 3;
console.log(history.state.test); // 3

// refresh the page

console.log(history.state.test); // 2

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.

@tbondwilkinson
Copy link
Contributor

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.

@domenic
Copy link
Collaborator Author

domenic commented Feb 11, 2021

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.

@domenic
Copy link
Collaborator Author

domenic commented Feb 11, 2021

To respond to

maybe I misunderstood the original post here, it seemed to suggest both removing object identity for Entries as well as changing the key on each mutation

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 key (stable, immutable) and id (changes on "mutation"). (Better names might be good...) But then we have a choice on how to model "mutation":

  • AppHistoryEntry object identity stays the same forever. Then we probably call the method update({ url, state }). In this world key is technically redundant, but probably still useful.

  • AppHistoryEntry object identity gets replaced on mutation. Then we probably call the method replace({ url, state }). In this world id is technically redundant, but probably still useful.

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.)

@Yay295
Copy link

Yay295 commented Feb 12, 2021

AppHistoryEntry object identity gets replaced on mutation. Then we probably call the method replace({ url, state }).

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 key because you could just keep a reference to the entry object itself.

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.

This was referenced Feb 12, 2021
@domenic
Copy link
Collaborator Author

domenic commented Feb 12, 2021

I moved the discussion on state immutability to #36.

@domenic domenic added the change A proposed change to the API, not foundational, but deeper than the surface label Feb 17, 2021
@tbondwilkinson
Copy link
Contributor

AppHistoryEntry object identity gets replaced on mutation. Then we probably call the method replace({ url, state }). In this world id is technically redundant, but probably still useful.

This is my preference because I don't want someone to think that they can use === to detect a URL change with a previous current history, when in fact a replace mutation might have occurred (and instead, I would prefer them to depend explicitly on the id field if they care about "slot" identity vs unique identity)

@domenic
Copy link
Collaborator Author

domenic commented Mar 30, 2021

If you do history.replaceState(newState), i.e. do not change the URL, should that change the AppHistoryEntry object identity and its id?

Certainly history.replaceState(newState, "", newURL) should update, but I'm less sure about window.history-only state changes impacting app history entries.

@domenic
Copy link
Collaborator Author

domenic commented Mar 30, 2021

Also, we never really answered: should id change if only state changes (and the URL stays the same)?

@tbondwilkinson
Copy link
Contributor

I do think it should change the AppHistoryEntry object identity and its id.

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

@Yay295
Copy link

Yay295 commented Mar 30, 2021

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.

@tbondwilkinson
Copy link
Contributor

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.

domenic added a commit that referenced this issue Apr 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change A proposed change to the API, not foundational, but deeper than the surface
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants