Incoming arg to updater cannot be modified
Closed this issue · 13 comments
When using the update function from ShiftDB, the prevState argument appears to be 'read-only'. I'm unaware of any spec which defines such functionality, so as of now I assume this is unintended.
update((someData = { key: [] }) => {
someData.key.push('item');
return someData;
});The above code results in the following error
TypeError: Cannot assign to read only property 'key' of #<Object>I suggest initially and immediately reverting this to the specced behavior, and then we can open a discussion regarding future design.
It appears that the behavior will be inconsistent: when using the default value it's going to be modifiable but when updating an existing value it is going to be modifiable.
I'm not sure when update passes a default value that is modifiable. I don't think the code shows that currently. But if it does , that's a bug.
We don't have a way to specify default values, instead default arguments are used - and they are regular objects and not read-only.
I think we see these differently. A default value is a return value. We do not put any requirements on these. To me it would be like saying that allowing () => ({ x: 17}) is wrong because it could be modified.
To be very clear: allowing updater modification of the value passed in precludes many (good) DB implementations. User code can always generate a deep copy if this is needed and the performance is appropriate. But it can never avoid having had this copy made before it was called.
We shall consider providing a low - performance alternative. It cannot be the default.
I am only referring to the provided example. If the database does not have a key the code will modify the array successfully, if the database has a value then it fails since it's read only.
Generally speaking:
If it is technically possible, I would like to give developers the develop experience they are used to.
We have (small) evidence that developers have run into this problem, and have tried to modify the original object - the contractor we got on Fiverr was stuck on this. Object on Javascript are not usually read only and we should not do things that are unusual, unless they carry significant user value.
Probably not an issue with a single spec - This might go into our design tenets.
For this instance:
Can we remove the constraint unless there is a critical technical reasons not to do so?
Would love to hear your thoughts if you think otherwise.
I think we should just allow modifying the original object and drop the read-only requirement to make the interface easier for devs to adopt.
There are some optimisations we can make if the object cannot be modified but I'm not sure it's worth it.
Just for the record, the idea was taken from a popular JS framework (immer).
Developers were supposed to write redux style reducers where the original value is never mutated or use something like immer to simplify working with immutable state.
React has readonly updaters, e.g. see the type of setState. We go a step further than that, and actively freeze these values. But changing a passed-in state seems very un-React-like.
immer.produce allows using the imperative style. Let's document it as a way to write imperatively!
OK, agreed we will make the object updatable (unfreeze it) and document best practice around using it.
I don't mind removing the actual freezing, (also, I think it's a must in production)
For all other means, this should be considered immutable to allow us to handle the data changes for the user more efficiently.
I would also like to stay consistent with React.setState
React Documentation
from: https://reactjs.org/docs/state-and-lifecycle.html#using-state-correctly
Do Not Modify State Directly
For example, this will not re-render a component:
// Wrong
this.state.comment = 'Hello';
Instead, use setState():
// Correct
this.setState({comment: 'Hello'});
The only place where you can assign this.state is the constructor.
So in react it's not allowed and frowned upon mutating the state.
The whole react mechanism works on the assumption that everything is immutable.
React with TypeScript
I wrote this code:
import React from 'react';
interface MyComponentProps {
}
interface MyComponentState {
something: string;
}
export default class MyComponent extends React.Component<MyComponentProps, MyComponentState> {
state = {
something: 'bla',
}
doSomething = () => {
this.state.something = 'wrong'; // No TypeScript warning here, though it's documented not to change this
this.setState((state) => {
state.something = 'else'; // (property) [something: string] Cannot assign to 'something' because it is a read-only property.ts(2540)
return state;
});
}
render() {
return <div>{this.state.something}<button onClick={this.doSomething}>bla</button></div>
}
}So leaving this object read only is perfectly consistent with React
Plan of record
@ashevat and @arielshaqed worked this out. It will be frozen once @ashevat approves, and then this comment will be appropriately marked and changed no more. The meme is not part of the specification; it is (at most) normative.
Users are choosers
Non-frozen is easy
declare async function update<T extends Serializable = any>(
key: string, updater: (state?: T) => T, options?: UpdateOptions,
): Promise<T>;The updater function type passed to update receives an unfrozen object and returns the desired new object (it still cannot be a procedure and return undefined, in order to allow users to code somewhat-functional updaters, e.g. to return a modified portion of the original object). update also returns a modifiable version of the object. Specifically, if there is any caching, update will perform copies.
This interface can also receive a strict updater (below).
Fast is possible
export async function updateStrict<T extends Serializable = any>(
key: string, updater: (state?: DeepReadonly<T>) => T, options?: UpdateOptions,
): Promise<DeepReadonly<T>> {The "strict" updater function type passed to updateStrict may not change its object and returns a new object. This interface is designed to allow fast updates, as update never has to copy.
In dev versions: The dynamic object type is frozen. If the updater function tries to modify it, updateStrict throws a specific Error. The message will include text specifying to change the updater function or to use update.
In prod versions: No runtime enforcement of strictness is performed.
LGTM, pending our launch and user feedback.