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

pass parent to default value function in t.optional #2050

Closed BrianHung closed 11 months ago

BrianHung commented 11 months ago

This PR allows the default value function in t.optional to access the parent context via an argument.

This is useful when you want the default value to be a function of the parent state. For example, if we had a user model and wanted to deterministically generate a unique color based on their userName, we could do the following:

import { types as t } from "mobx-state-tree" 
const User = t.model("User", {
  id: t.identifier,
  userName: t.string,
  color: t.optional(t.string, (snapshot) => hashStringToColor(snapshot.userName))
})

const user = User.create({ id: uuid(), userName: "brian" });
console.log("color", user.color);

Right now, there isn't a way to generate the default value based on some state node.

Test cases would probably look like the situation above, plus an additional test case checking that the argument is null when the type is not a direct child of a model type.

const color = t.optional(t.string, snapshot => {
  console.log("snapshot", snapshot == null)
  return "#000";
});

color.create()
coolsoftwaretyler commented 11 months ago

Hey @BrianHung - kind of like in #2049 - would you mind at least updating the description here to give me/future readers a sense of the purpose of this PR? I can understand the code changes in isolation, but I don't fully get the context of how we might use this change, test it, validate it, etc.

Even an updated description would work, but you know I'll be stoked for a few test cases, some kind of example code, etc., haha ;)

Tag me when you've updated things and I'll take a look!

BrianHung commented 11 months ago

@coolsoftwaretyler Thanks for the feedback; hope the edit helps!

coolsoftwaretyler commented 11 months ago

Thanks @BrianHung - that's really helpful.

This looks good and CI is passing. I'm going to merge it in. I think it should land in the next minor release, although I'm unsure when that'll be going out.

Really appreciate the PR and the back-n-forth.

chakrihacker commented 10 months ago

@coolsoftwaretyler I feel we should not allow users to have a snapshot of the model, inside optional. The correct way is to have a views where it can derive the state from the existing model.

import { types as t } from "mobx-state-tree" 
const User = t.model("User", {
  id: t.identifier,
  userName: t.string,
}).views(self => {
  return {
    color: () => {
      return hashStringToColor(self.userName)
    }
  }
})
coolsoftwaretyler commented 10 months ago

@chakrihacker - I don't think your code snippet solves the use case @BrianHung is looking for.

The purpose of this PR is to allow optional values to look up at their parent while determining their default values.

Can you say more about your objection to the addition? What specifically are you concerned about?

chakrihacker commented 10 months ago

@coolsoftwaretyler I am talking about this statement

if we had a user model and wanted to deterministically generate a unique color based on their userName

This statement means a derivation from existing model data, views are used to derive values from existing data

If you are ok I can explain better over a call

coolsoftwaretyler commented 10 months ago

@chakrihacker - you're correct that a view is a good use for computed values like that, but I can also see the need for adding parent context for types.optional.

Views don't end up in snapshots, but a types.optional with a default value would.

If we need to we can do a call, but I'd rather discuss async first.

Can you say more specifically what concerns you about this PR? My opinion is it gives tools to developers, but they don't have to use it.

It could also be good to hear from @BrianHung about any other reason that views wouldn't solve his requirements.

chakrihacker commented 10 months ago

I was writing test cases for this and had a thought that why haven't we used views for this, so raising question

coolsoftwaretyler commented 10 months ago

Gotcha. I think we should wait and let Brian talk a little more about the use case, but I think there are a few scenarios where a view wouldn't solve the problem.

BrianHung commented 10 months ago

This statement means a derivation from existing model data, views are used to derive values from existing data

Color should only be initially derived from the existing username when it is undefined, and thereafter not be derived. That's why it is not a view and makes more sense to be in the optional default value function.

For example,

import { types as t } from "mobx-state-tree" 
const User = t.model("User", {
  id: t.identifier,
  userName: t.string,
  color: t.optional(t.string, (snapshot) => hashStringToColor(snapshot.userName))
}).actions(self => ({
  setUserName(name) {
    self.userName = name
  },
  setColor(color) {
    self.color = color
  },
}))
chakrihacker commented 10 months ago

@BrianHung What about preProcessSnapShot doesn't that help your use case??

import { types as t } from "mobx-state-tree" 
const User = t.model("User", {
  id: t.identifier,
  userName: t.string,
  color: t.optional(t.string, null)
}).preProcessSnapShot(snapshot => ({
  color: snapshot.color ?? hashStringToColor(snapshot.userName)
})).actions(self => ({
  setUserName(name) {
    self.userName = name
  },
  setColor(color) {
    self.color = color
  },
}))
BrianHung commented 10 months ago

@chakrihacker Maybe the example I gave was too basic, since I agree that preProcessSnapshot solves the problem in this example. Let's change the situation and say that userName is stored elsewhere in the state tree, or that color should be a function of some sibling or ancestor state in the state tree.

Then you would need getRoot or getParent.

color: t.optional(t.string, (snapshot, parent) => getRoot(parent)...

or this PR where you can access the parent node in the preProcessSnapshot to solve it. https://github.com/mobxjs/mobx-state-tree/pull/2065