-
-
Notifications
You must be signed in to change notification settings - Fork 589
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
Discussion: atom values should be freezed/immutable #167
Comments
oops. |
Do you agree the issue exists even with |
I agree that if some object is shared between components, it's change changes all components. But because callbacks with updated value are used - UI is still valid. Let's say I have that array of tags. If tag value is changed (isSelected = true) it doesn't cause a rerender. |
If atom is changed inherently, there is no notification of that, we don't know that it's happened. And if |
If I'm correct Recoil has immutable atom values. |
I'm not sure if I understand this.
But, I get these two.
Hm, wonder if they deep freeze it. |
I would like to revisit this: #41 (comment) const updatedTags = produce(Array.from(taskTags), (draft) => {
const tag = draft.find((it) => it.id === action.id);
if (tag !== undefined) {
tag.isSelected = action.isSelected;
}
});
console.log(
`taskTagsAtom: updatedTags are ${JSON.stringify(updatedTags, null, 2)}`
); Why on earth immer goes wrong? |
Frankly speaking I don't know. From my understanding this is totally valid code. But |
But I use Hermes engine in React Native. It might lack some of API. Also, since in my Hermes version no Proxy support, I use |
Does removing Array.from help? - const updatedTags = produce(Array.from(taskTags), (draft) => {
+ const updatedTags = produce(taskTags, (draft) => {
I see. that can be something. |
No, didn't help. Changing |
I was really bothered by this. I wasn't able to understand why immer wasn't doing it's job. So I replicated OP problem on codesandbox but for some reason, the code works normally as intended. The original The problem might be on the instanced array that populated the const tagRepository = getTagRepository();
return tagRepository.find(); |
It returns @Entity({ name: 'tags' })
export class Tag {
@PrimaryGeneratedColumn('uuid')
id!: string;
@Column('text')
label!: string;
@Column('text')
iconName!: IconName;
@Column('text')
iconColor!: string;
@Column({ type: 'boolean', default: Number(false) })
isSelected!: boolean;
} It's populated by TypeORM. |
I get it, thank you so much for pointing out to the right direction! + import { immerable } from 'immer';
@Entity({ name: 'tags' })
export class Tag {
+ [immerable] = true;
@PrimaryGeneratedColumn('uuid')
id!: string;
@Column('text')
label!: string;
@Column('text')
iconName!: IconName;
@Column('text')
iconColor!: string;
@Column({ type: 'boolean', default: Number(false) })
isSelected!: boolean;
} solves the problem 😄 at least for this producer const updatedTags = produce(Array.from(taskTags), (draft) => {
const tagIndex = draft.findIndex((it) => it.id === action.id);
if (tagIndex >= 0) {
// Found tag with this id
draft[tagIndex].isSelected = action.isSelected;
}
}); |
Nice. I was suspecting you were using a class. But I thought it was weird for this to happen. I wasn't even aware of |
@italodeandra Nice comments! Now, back to the original discussion, I'm not a big fan of adding deep-freeze or any kind in core. Let's see how this works: const useFrozenAtom = (anAtom) => {
const [value, setValue] = useAtom(anAtom)
return useMemo(() => [deepFreeze(value), setValue], [value, setValue])
} const freezeAtom = (anAtom) => {
const frozenAtom = atom(
get => deepFreeze(get(anAtom)),
(get, set, arg) => set(anAtom, arg)
)
return frozenAtom
} Would anyone be interested in trying? |
@dai-shi I was wondering how it is implemented in Recoil. if (__DEV__) {
if (options.dangerouslyAllowMutability !== true) {
deepFreezeValue(newValue);
}
} I think this might be an option. |
Yeah, that's one option for sure. Let me propose another one which I think is to keep our code simple. import deepFreeze from 'deep-freeze'
<Provider freeze={deepFreeze}> |
Hm, I noticed Recoil_atom.js is atom.ts in our case, not Provider. So, this should be another option: import { atom as origAtom } from 'jotai'
export const atom = (read, write) => {
const anAtom = origAtom(read, write)
const origRead = anAtom.read
anAtom.read = get => deepFreeze(origRead(get))
return anAtom
} |
Would you like to try #171 and see if it's satisfying? |
if (freeze) {
// XXX this is not very efficient
console.log(`state: ${JSON.stringify(state)}`)
mKeys(state).forEach(function (a) {
var aState = mGet(state, a);
console.log(`aState: ${typeof aState.value}`)
freeze(aState.value);
});
} [Sun Nov 08 2020 12:55:56.795] LOG state: [{}]
[Sun Nov 08 2020 12:55:56.807] LOG aState: undefined
While without |
@likern Thanks for trying! % node
> Object.freeze(undefined)
undefined
> Object.getOwnPropertyNames(undefined)
Uncaught TypeError: Cannot convert undefined or null to object
at Function.getOwnPropertyNames (<anonymous>) Oh, no. It was unexpected, sorry. |
fc64847 |
Now, let's try another approach. const freezeAtom = (anAtom) => {
const frozenAtom = atom(
get => deepFreeze(get(anAtom)),
(get, set, arg) => set(anAtom, arg)
)
return frozenAtom
} Does this work? https://codesandbox.io/s/react-forked-mdmic?file=/src/App.js Okay, this seems fine. Now, I prefer to avoid having deepFreeze in core. |
Based on discussion in #41
The text was updated successfully, but these errors were encountered: