-
Notifications
You must be signed in to change notification settings - Fork 30
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
App history state (im)mutability #36
Comments
Things that don't workNote that we can't use const frozenSet = Object.freeze(new Set());
// Still works
frozenSet.add(2); Only allow immutable primitivesOnly allow immutable primitives (numbers, strings, booleans, bigints, symbols, etc.). No objects allowed. We could make this slightly less painful by having the // Auto-convert object literals to entries in the app history state map
appHistory.push({ state: { test: 2 } });
console.log(appHistory.current.state.get('test')); // 2
appHistory.current.state.set('test', 2);
console.log(appHistory.current.state.get('test')); // 3
// refresh the page
console.log(appHistory.current.state.get('test')); // 3 In the future, if records and tuples ever happen, they would also be allowed, which would give you the ability to contain larger object graphs. I suspect this would be fairly constraining for web developers, and would push them toward using app history state to store cache keys or similar which then have to be looked up in out of band sources like Try to more aggressively snapshot the state?We could imagine trying to serialize the current value of appHistory.push({ state: { test: 2 } });
console.log(appHistory.current.state.test); // 2
appHistory.current.state.test = 3;
console.log(appHistory.current.state.test); // 3
// refresh the page: browser snapshots current value of state.
console.log(appHistory.current.state.test); // 3, it works!
appHistory.current.state.test = document.body;
// refresh the page: browser tries to snapshot current value of state,
// but fails because you can't serialize a DOM node
console.log(appHistory.current.state.test); // ??? probably throws an exception because appHistory.current.state is null? |
Instead of having I suppose there could also be a Having the |
I do think: appHistory.getCurrent().state or appHsitory.current.getState() are compelling minor changes to communicate to users that the state Object cannot be mutated in place, and that changes to it will not affect other calls to |
Just changing it to a function could definitely help. However, that would work best if we re-cloned the data upon each access, i.e.: Return a new clone on every accessconst stateInput = { test: 2 };
appHistory.push({ state: stateInput });
const state1 = appHistory.current.state();
console.assert(stateInput !== state1);
const state2 = appHistory.current.state();
console.assert(state1 !== state2); This kind of behavior makes the situations from the OP very clear then: appHistory.push({ state: { test: 2 } });
console.log(appHistory.current.state().test); // 2
appHistory.current.state().test = 3; // modifies a throwaway object
console.log(appHistory.current.state().test); // still 2
// refresh the page
console.log(appHistory.current.state().test); // 2 I wonder if this kind of cloning if that would be too expensive, or would be OK? It kind of depends on how large the state is that people store here, and how often they access it, which I don't have a good intuition for. |
Would it be a deep clone or a shallow clone under this proposal? |
Deep clone, in particular a structured clone |
I actually want to push back on this:
In today's history API, history.state does serialization to raw Objects, like JSON (but not JSON), that means that you cannot have an Object with function properties:
So I think we could have a deeply immutable state Object without having to worry about mutability with something like methods. So I think my preference would definitely be a state Object that gets serialized without methods or prototypes, basically just supporting Object literals, arrays, and primitives. |
And another note is that history.pushState actually errors of the properties of an Object cannot be cloned, e.g.:
|
The code examples you show exemplify how the current history API snapshots the passed-in value. (Using structured clone.) They don't show the passed-in value being immutable, though. It's making object graphs immutable which is not really possible in JavaScript; snapshotting them is fine. |
Can you explain a little more why it's not possible to make the Object graph immutable? I would argue that if it's serializable by the history API today, it's possible to make it deeply immutable |
Yeah, "not possible" is overstating it. But I think the connection between snapshotting and immutability is pretty weak. The only thing they share in common is that they both have to walk the object graph. What I think you're getting at is something like: Custom snapshot and semi-deep freeze
I say "semi" deep freezes because we wouldn't freeze their prototype chains, because it could contain things like Object.prototype.test = 2;
history.pushState({ }, 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); // undefined but I don't think we're really concerned about that case. My main concern with such an operation is that it'd be rather un-idiomatic and unprecedented for the web platform. I like "Only allow immutable primitives" or "Return a new clone on every access" a good bit better from that perspective... |
Well, that example is a bit misleading because the serialized representation itself does not serialize the I'm also fine with 'return a new clone on every access' but your point about performance does give me pause. But for another perspective, we store basically the max allowable size in session storage and do frequent deserializations (we store a string representation of data), so maybe it's not so bad after all. But if we go down this route, we may want to consider some way to segment the data so that if you do end up having a very large data object you want to store in state, you could read a portion of that from the state Object without having to deserialize the entire Object. But then maybe we can just say "If you have performance issues, you should be using session storage to link to your history key, rather than history state itself" What does "only allow immutable primitives" mean? |
I was referrring to the solution proposed in #36 (comment) |
I agree with your assessment that without language support for immutable Objects or record/tuple that's a pretty hard sell in terms of restrictiveness. I don't like any of our options much here, but I think the best option is probably structured clone. I'll dream of a day when JavaScript supports immutable types (I imagine, if you passed in immutable Objects into the appHistory APIs, that immutableness would be preserved, so there is some forward compatibility with that direction) |
To confirm, you mean Return a new clone on every access, not Custom snapshot and semi-deep freeze, right? |
Correct |
With this in place, I think we should add an explicit |
Closes #36 by moving from appHistoryEntry.state to getState() and setState() methods. This adds the ability to modify the state of non-current entries as a bonus. Solves much of #42 by making push() and update() navigate by default, although it does not yet include the initiator idea. As part of this we simplify update() significantly, since now if you want to change state without performing a navigation, you would use setState(). This makes the URL argument required for it. For symmetry, I removed url from the options for push().
Closes #36 by moving from appHistoryEntry.state to getState() and setState() methods. This adds the ability to modify the state of non-current entries as a bonus. Solves much of #42 by making push() and update() navigate by default, although it does not yet include the initiator idea. As part of this we simplify update() significantly, since now if you want to change state without performing a navigation, you would use setState(). This makes the URL argument required for it. For symmetry, I removed url from the options for push().
Closes #36 by moving from appHistoryEntry.state to getState() and setState() methods. This adds the ability to modify the state of non-current entries, as well as a statechange event, as a bonus. Solves much of #42 by making push() and update() navigate by default, although it does not yet include the initiator idea. As part of this we simplify update() significantly, since now if you want to change state without performing a navigation, you would use setState(). This makes the URL argument required for it. For symmetry, I removed url from the options for push().
Continuing from #7 (comment), where @tbondwilkinson notes a sharp edge of the existing
history.state
API:That is, because it allows any arbitrary JavaScript object, it allows mutable JavaScript objects. But the object is only serialized into (semi-)persistent storage when you call
history.pushState()
orhistory.replaceState()
.The current proposal duplicates this sharp edge with app history state. E.g.,
This seems quite bad to me, and I think we should consider a way to fix it, if we can do so without interfering with other use cases. I'll post a reply with some suggestions.
The text was updated successfully, but these errors were encountered: