search

immerjs / immer

Create the next immutable state by mutating the current one
https://immerjs.github.io/immer/
MIT License
27.67k stars 847 forks source link

Set and Map implementations break Safari structuredClone #1149

Open SKaistrenko opened 3 days ago

SKaistrenko commented 3 days ago

🐛 Bug Report

A clear and concise description of what the bug is.

If your bug report involves classes, check the docs on how classes are handled first. For patches, don't file issues if they are not optimal, only if they are incorrect.

Link to repro

A bug report without a reproduction is not a bug report. Failing to follow this templately is likely to result in an immediate close & lock of the issue.

https://codesandbox.io/p/sandbox/immer-sandbox-forked-nf577m?file=%2Fsrc%2Findex.ts%3A6%2C8&workspaceId=76a181a1-ec65-4f80-815c-e379a177be13

Unfortunately CodeSandbox doesn't support Safari well - you'll need to run manually on Safari. On the link above you can see code work ok in Chrome

To Reproduce

Steps to reproduce the behaviour: In Safari browser Call structuredClone with either a Set or a Map passed through immer

The above op is called within indexedDB put operation (MDN docs)

Observed behaviour

DataCloneError: The object can not be cloned., value: [object Object]

Expected behaviour

No error - as with plain Map and Set that work well.

Environment

Latest immer, Safari

oriSomething commented 1 day ago

I don't think it's related to Safari. Proxy seems not be possible to be clone with structuredClone

I've tried running this in Chrome which result in the same error:

x = new Map([[1,1]]); structuredClone(new Proxy(x, {})) === x;
// throws: Uncaught DataCloneError: Failed to execute 'structuredClone' on 'Window': #<Map> could not be cloned.
mweststrate commented 1 day ago

This looks to me as working as intended, drafts are very special objects and should never leave the produce function by any other means than returning it. Why are you structuralCloning those? The output of immer is immutable anyway, so that seems completely unnecessary?

On Fri, Oct 25, 2024, 12:17 PM Ori Livni @.***> wrote:

It's not related to Safari. Proxy seems not be possible to be clone with structuredClone

I've tried running this in Chrome which result in the same error:

x = new Map([[1,1]]); structuredClone(new Proxy(x, {})) === x;// throws: Uncaught DataCloneError: Failed to execute 'structuredClone' on 'Window': # could not be cloned.

— Reply to this email directly, view it on GitHub https://github.com/immerjs/immer/issues/1149#issuecomment-2438626999, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAN4NBDQNJRI2DKJKPLDUYLZ5KKLLAVCNFSM6AAAAABQNRVGIWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMZYGYZDMOJZHE . You are receiving this because you are subscribed to this thread.Message ID: @.***>

mweststrate commented 1 day ago

Note that there is also current if really needed if you want to leak a snapshot, although using it is usually a code smell imho https://immerjs.github.io/immer/current

On Fri, Oct 25, 2024, 12:39 PM Michel Weststrate @.***> wrote:

This looks to me as working as intended, drafts are very special objects and should never leave the produce function by any other means than returning it. Why are you structuralCloning those? The output of immer is immutable anyway, so that seems completely unnecessary?

On Fri, Oct 25, 2024, 12:17 PM Ori Livni @.***> wrote:

It's not related to Safari. Proxy seems not be possible to be clone with structuredClone

I've tried running this in Chrome which result in the same error:

x = new Map([[1,1]]); structuredClone(new Proxy(x, {})) === x;// throws: Uncaught DataCloneError: Failed to execute 'structuredClone' on 'Window': # could not be cloned.

— Reply to this email directly, view it on GitHub https://github.com/immerjs/immer/issues/1149#issuecomment-2438626999, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAN4NBDQNJRI2DKJKPLDUYLZ5KKLLAVCNFSM6AAAAABQNRVGIWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMZYGYZDMOJZHE . You are receiving this because you are subscribed to this thread.Message ID: @.***>

SKaistrenko commented 1 day ago

TLDR: In my understanding immer modifications to original data are leaking out beyond RTK/produce.

We are not trying to run structuredClone within produce - this just seems the most straightforward repro. We use Redux RTK and some of the data we're storing includes Set. After that we're selecting this data within the app cycle and do a buck-up using indexed DB.

It is my understanding that immer is by reference modifying the underlying objects we're storing, resulting in the described behaviour.

I'm able to confirm the above assumption by exporting the Set in question and checking the === before putting into IndexedDB. Meaning

const someSet = new Set();
const initialState = { blah: someSet };
...

const stateData = <selectState>;
someSet === stateData.blah // true

If I log the Object.keys of the above set before the RTK slice logic, I get an empty list In the data I select the above return `[add, delete, ]

markerikson commented 1 day ago

@SKaistrenko my first observation is that you shouldn't be putting Sets into Redux state anyway:

mweststrate commented 1 day ago

It is my understanding that immer is by reference modifying the underlying objects we're storing

That understanding is wrong. Not doing that is kinda the point of the whole library (as covered on https://immerjs.github.io/immer/ frontpage :)) draft objects are mutable, yes, but neither the inputs or outputs of produce are. So draft objects (they are temporarily mutable copies) behave entirely different from what you get back from produce.

So in either case structuredClone seems to be a strange thing to use icm with immer. If you structureClone the output of produce, that is moot since you could just directly have handed out the reference itself since it is immutable. If you do it inside produce, you are leaking objects that are temporal and this is by design a thing that you should't be doing (or needing. Why would you even share a ref to an object in the middle of some update process?). Just as you wouldn't be sharing the input argument of a react useState updater function with anything that outlives that very function.

SKaistrenko commented 1 day ago

@SKaistrenko my first observation is that you shouldn't be putting Sets into Redux state anyway:

Agreed

If you do it inside produce, you are leaking objects that are temporal and this is by design a thing that you should't be doing

The only reason I exported that reference is to validate my assumption that immer messed with the original ref - which doesn't sound ideal.