Closed BrianHung closed 10 months ago
Looks great, thanks @BrianHung!
I'm gonna merge this in. Might end up just wrapping it up in a new candidate for 5.2.0 this weekend.
The posted example above doesn't work as is https://codesandbox.io/s/mobx-state-tree-pr-2065-8kj95v
@chakrihacker - you have that pinned to 5.2.0-alpha.1
, which doesn't include this PR. I think you'll have to test it out with a local build of the master
branch.
In Mobx State Tree when you mark something as required, you validate before creating that instance, something like this
https://codesandbox.io/s/mobx-state-tree-pr-2065-with-working-8f3qgp?file=/src/index.js
I don't see any use case here too
@chakrihacker - two questions for you:
My philosophy when merging this is that it's a small change with a totally opt-in feature, and I know Brian has a lot of advanced use cases that I want to support in the main MST releases. I don't think this PR impacts stability or performance, so I was happy to give him some nicer utilities than we previously had.
@chakrihacker - you have that pinned to
5.2.0-alpha.1
, which doesn't include this PR. I think you'll have to test it out with a local build of themaster
branch.
The problem is not about the master, you cannot just directly push to the array, without actions
Calculating parent is definitely a performance issue.
The problem is this doesn't contain proper test cases and documentation and yet it's merged.
I am not against new features here, I am just trying to help and validate that my most loving state management library is working properly.
@chakrihacker - the missing test cases and documentation is a fair point.
My approach here was to be grateful for Brian's work as is, since the change was small and I know he's wanted things like this in the past. I was hoping to keep things moving and extend functionality for a heavy user in a small way. Perhaps I moved a little too quickly here.
However, this is precisely the reason I wanted to start with an alpha build for 5.2.0
, and why we haven't pushed this change directly to the npm package. It gives us time to address these issues.
Here's my proposal for how to resolve this:
master
.In the meantime, we'll hold off on 5.2.0
releases. If we need to cut patch releases for what's out there, we can. Worst case, if this process takes too long, or if we find issues with the PR as is that can't be resolved, we can revert the change and work to find another path forward.
In parallel, we should consider updating our contributing guidelines. We don't currently have a set of rules about what PRs can and can't be merged, so it's up to individual judgment. In this case, I perhaps made a poor judgment call.
I'd be happy to take a draft set of contribution guidelines from you, or input from you as well. My plan would be to start with a similar set library's guidelines.
All that to say, thanks for the feedback, and apologies for the misstep here. We will figure out a path forward!
Thanks @coolsoftwaretyler Apart from Performance implications there is also other things we need to consider here, learning curve, mental model. @BrianHung I am not at all against your contributions here and if it helps MST I am also one of the happy user.
This PR gives
preProcessor
andpostProcessor
access to the parent context.This is useful when the incoming / outgoing snapshot should depend on parent state. For example, let's say we have an array of objects that each have a name and a type. When
snapshot.name === null || snapshot.name === ""
, we want to use${type} ${number of types in array + 1}$
.With access to the parent context in
preProcessor
, we can do this easily!