mobxjs / mobx-state-tree

Full-featured reactive state management without the boilerplate
https://mobx-state-tree.js.org/
MIT License
6.9k stars 640 forks source link

Unexpected error "Сannot finalize the creation of a node that is already dead" while applying snapshot #2136

Open dfilatov opened 5 months ago

dfilatov commented 5 months ago

Bug report

Sandbox link or minimal reproduction code

import { types, applySnapshot } from 'mobx-state-tree';

const Model = types.array(
    types.model({
        b: types.string,
        c: types.optional(
            types.model({
                d: types.snapshotProcessor(
                    types.model({ e: '' }),
                    {
                        preProcessor(e: string) {
                            return { e };
                        },

                        postProcessor(snapshot) {
                            return snapshot.e;
                        }
                    }
                )
            }),
            { d: '' }
        )
    })
);

const m = Model.create([{ b: 'b' }]);

applySnapshot(m, []);

Describe the expected behavior

Should work without any errors

Describe the observed behavior

Error: [mobx-state-tree] assertion failed: cannot finalize the creation of a node that is already dead

coolsoftwaretyler commented 5 months ago

I think this may be a docs and TypeScript issue. You're calling preProcessor and postProcessor, but the types.snapshotProcessor actually wants preProcess and postProcess.

You can see it working here: https://codesandbox.io/p/sandbox/issue-2136-7hpyd2?file=%2Fsrc%2Findex.ts%3A21%2C3

But there's TypeScript errors on those actions.

Here's the working code for posterity:

import { types, applySnapshot } from "mobx-state-tree";

const Model = types.array(
  types.model({
    b: types.string,
    c: types.optional(
      types.model({
        d: types.snapshotProcessor(types.model({ e: "" }), {
          // TypeScript errors here, but I think that's incorrect
          preProcess(snapshot: { e: string }) {
            return { e: snapshot.e };
          },

         // TypeScript errors here, but I think that's incorrect
          postProcess(snapshot: { e: string }) {
            return snapshot.e;
          },
        }),
      }),
      { d: { e: "e" } }
    ),
  })
);

const m = Model.create([{ b: "b" }]);

applySnapshot(m, []);

I'll tag this, and please accept our apologies for the bug! We'll close this issue when we fix the types, but please let me know if I've missed anything else.

dfilatov commented 5 months ago

@coolsoftwaretyler In your version preProcess/postProcess aren't even called. Not sure you're right about naming: https://github.com/mobxjs/mobx-state-tree/blob/master/src/types/utility-types/snapshotProcessor.ts#L60 https://github.com/mobxjs/mobx-state-tree/blob/master/src/types/utility-types/snapshotProcessor.ts#L75

coolsoftwaretyler commented 5 months ago

Ah, my bad! I had a false positive, was just getting those errors to go away.

There is a documentation issue. We call it preProcess and postProcess here: https://mobx-state-tree.js.org/overview/hooks#snapshot-processing-hooks. That's what threw me and got me here.

I'll remove the TypeScript label here. I've been messing around with the code and I think this is coming from postProcessor. If you remove that action, the error goes away.

coolsoftwaretyler commented 5 months ago

Seems to be related to applying the snapshot as an empty array. I can get the processed snapshot, modify it in place, and apply it correctly:

import { types, applySnapshot, getSnapshot } from "mobx-state-tree";

const Model = types.array(
  types.model({
    b: types.string,
    c: types.optional(
      types.model({
        d: types.snapshotProcessor(types.model({ e: "" }), {
          preProcessor(snapshot: { e: string }) {
            return { e: snapshot.e };
          },

          postProcessor(snapshot: { e: string }, node) {
            return {
              ...snapshot,
              e: "modified",
            };
          },
        }),
      }),
      { d: { e: "e" } }
    ),
  })
);

const m = Model.create([{ b: "b" }]);

const snapshot = getSnapshot(m);

console.log("snapshot", snapshot);

const modifiedSnapshot = [...snapshot];

console.log("modifiedSnapshot", modifiedSnapshot);

modifiedSnapshot[0].c.d.e = "modifiedAgain";

applySnapshot(m, modifiedSnapshot);

console.log("snapshot one last time", getSnapshot(m));

Updated example there:

https://codesandbox.io/p/sandbox/issue-2136-7hpyd2?file=%2Fsrc%2Findex.ts%3A41%2C1

But if you try to apply as an empty array, the error comes back.

dfilatov commented 5 months ago

Sorry, I can't open your example:

image
coolsoftwaretyler commented 5 months ago

Whoops, getting used to the new CodeSandbox tools. This should work: https://codesandbox.io/p/sandbox/issue-2136-7hpyd2?file=%2Fsrc%2Findex.ts%3A41%2C1

coolsoftwaretyler commented 5 months ago

I've got some pass-by-reference errors in there, but I gotta run at the moment so I can revisit this later. Sorry 'bout that!

dfilatov commented 5 months ago

It's not related with an empty array, following code throws the same error:

const Model = types.array(
      types.model({
        b: types.string,
        c: types.optional(
          types.model({
            d: types.snapshotProcessor(types.model({ e: '' }), {
              preProcessor(e: string) {
                return { e };
              },

              postProcessor(snapshot) {
                return snapshot.e;
              }
            })
          }),
          { d: 'e' }
        )
      })
    );

    const m = Model.create([{ b: 'b' }, { b: 'bb' }]);

    applySnapshot(m, [{ b: 'b' }]); // error

Interestingly, if I remove types.optional in the middle then everything works:

    const Model = types.array(
      types.model({
        b: types.string,
        c:
          types.model({
            d: types.snapshotProcessor(types.model({ e: '' }), {
              preProcessor(e: string) {
                return { e };
              },

              postProcessor(snapshot) {
                return snapshot.e;
              }
            })
          })
      })
    );

    const m = Model.create([{ b: 'b', c: { d: '' } }, { b: 'bb', c: { d: '' } }]);

    applySnapshot(m, [{ b: 'b', c: { d: '' } }]);
coolsoftwaretyler commented 5 months ago

Interesting. Maybe types.optional is having trouble instantiating the default value as a snapshotProcessor.

coolsoftwaretyler commented 5 months ago

Hopefully it's just this: https://github.com/mobxjs/mobx-state-tree/pull/2137

Once we get a test in there I can merge and deploy a patch, probably early this week.

dfilatov commented 5 months ago

Unfortunately, no ( It just fixes import but exception will be thrown anyway, I've checked it.

coolsoftwaretyler commented 5 months ago

Ah dang. Do you have the full text of the exception with that patched? That might be helpful

dfilatov commented 5 months ago

Ah dang. Do you have the full text of the exception with that patched? That might be helpful

image
coolsoftwaretyler commented 4 months ago

Thank you! I'll see if I can get anyone to investigate. If not, I can at some point in the coming weeks. Would welcome a PR if you trace the error on your own as well. Apologies for the inconvenience!

dfilatov commented 2 months ago

Hi, @coolsoftwaretyler! Is there any progress?

coolsoftwaretyler commented 2 months ago

Sorry, not yet! Haven't had anyone volunteer to work on it.