mobxjs / mobx-state-tree

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

add test cases for optional #2067

Closed chakrihacker closed 10 months ago

chakrihacker commented 10 months ago

What does this PR do and why?

Add test cases for optional type, where it can access snapshot of the parent

Steps to validate locally

Run the test cases

closes #2056

chakrihacker commented 10 months ago

@BrianHung Can you check this code? For some reason, I am getting snapshot as undefined Not sure what's the mistake

You can cd into mobx-state-tree and run this command yarn jest -t "access parent state"

Also even when I modify the model to this, I am not able to get snapshot

 const TodoModel = types.model({
        id: types.identifier,
        name: types.string,
        priorityModel: types.optional(types.model({
          // @ts-ignore
          priority: types.optional(types.string, (snapshot) => {
              console.log({ snapshot })
              if (snapshot) {
                  if (snapshot.name.toLowerCase().includes("today")) {
                      return "High"
                  }
              }
              return ""
          })
        }), {})
    })
chakrihacker commented 10 months ago

@coolsoftwaretyler Can you help me here, why snapshot is undefined ??

coolsoftwaretyler commented 10 months ago

@chakrihacker - did you run yarn build at the top level of the repo before this? cd packages/mobx-state-tree && yarn jest -t "access parent state" is working from me after doing that.

BrianHung commented 10 months ago

@coolsoftwaretyler @chakrihacker

After diving deeper into mobx-state-tree's architecture, I think we should actually revert these two PRs. https://github.com/mobxjs/mobx-state-tree/pull/2050 https://github.com/mobxjs/mobx-state-tree/pull/2065

I had initially written these PRs to give the snapshotProcessor and optional types access to the parent context upon initialization, similar to how the reference type gives access to parent in the custom get and set options.

However, the two PRs above in their current implementations can't access the parent StateTreeNode. This is because with MST and lazy initialization, the parent isn't created until after the child nodes are initialized.

A possible solution would be maybe to optimistically create the parent, which is what references do, or give to the parent ObjectNode instead where users could call getValue to create the parent.

However, with the first solution, reference types require parents to work while snapshotProcessors and optional don't have the same expectations; and with the second solution, AnyObjectNode is an internal utility currently not exposed (and probably shouldn't) and would also prevent lazy initialization at the cost of performance.

This doesn't mean there aren't any solutions. As @chakrihacker pointed out in a comment, the solution to needing parent context in optional, you can use a snapshotProcessor; and if you wanted parent context in either, you could use actions or helper functions.

.actions(parent => ({
  createChild(props) {
    return ChildType.create({
     ...createProps(props, parent)
    })  
}
}));
function createChildType(props, parent) {
  return ChildType.create({
     ...createProps(props, parent)
  })  
}

The main takeaway is, if you need to access the parent node when creating the child type, the child props have to be derived from the parent / outside context first and then passed into create, as Mobx-State-Tree optimistically creates the child snapshot before initializing the parent state tree node.

coolsoftwaretyler commented 10 months ago

Hey @BrianHung - thanks so much for staying on top of this and doing the extra work to re-think and catch that!

@chakrihacker - thank you as well for digging in on these changes and prompting all of this work to make sure we keep MST reliable. I think we can close this PR, and I will work on some additional PRs to revert the changes.

I'll close out some of the related issues as well.

coolsoftwaretyler commented 10 months ago

Revert PR incoming: https://github.com/mobxjs/mobx-state-tree/pull/2074

I'll have this out in the next release candidate sometime in a couple hours.