add parent to preProcessor and postProcessor

[BrianHung]

This PR gives preProcessor and postProcessor 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!

import { types as t } from "mobx-state-tree" 

const Shape = t.model("shape", {
  type: t.string,
  name: t.string,
})

const ShapeWithIncrementingName = t.snapshotProcessor(Shape, {
    preProcessor(snapshot, parent) {
        if (parent && (snapshot.name === null || snapshot.name === "") {
           const num = parent.filter(s => s.type === snapshot.type).length;
           snapshot.name = `${snapshot.type} ${num + 1}`
        }
        return snapshot
    },
});

const Shapes = t.array(Shape);
const shapes = Shapes.create([]);

shapes.push({ type: "rectangle"})
shapes.push({ type: "circle"})
shapes.push({ type: "rectangle"})

console.log(shapes[0]) // { type: "rectangle", name: "rectangle 1"}
console.log(shapes[1]) // { type: "rectangle", name: "circle 1"}
console.log(shapes[2]) // { type: "rectangle", name: "rectangle 2"}

[coolsoftwaretyler]

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.

[chakrihacker]

The posted example above doesn't work as is https://codesandbox.io/s/mobx-state-tree-pr-2065-8kj95v

[coolsoftwaretyler]

@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.

[chakrihacker]

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

[coolsoftwaretyler]

@chakrihacker - two questions for you:

1. What do you think of the original code snippet posted in this description? Is there a different way you think it should be solved?
2. Do you have any specific concerns about the PR itself? Let's say you never used this feature that Brian added, does this code introduce regressions, performance concerns, or some other problem to MST that you want to guard against?

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]

@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.

The problem is not about the master, you cannot just directly push to the array, without actions

[chakrihacker]

Calculating parent is definitely a performance issue.

The problem is this doesn't contain proper test cases and documentation and yet it's merged.

[chakrihacker]

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.

[coolsoftwaretyler]

@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:

1. I think the fact that Brian is asking for this is a strong signal that there are valid use cases here. Like I said, he has pretty advanced use cases and I trust his judgment. I'd like to see if we can make this PR viable in master.
2. Let's work together to put together some test cases here and make sure they work and make sense. I'd love to have some input from @BrianHung on what those could look like.
3. Once we have test cases in place, we can write up some documentation.
4. If we're still concerned about performance implications, I want to measure that and validate it.

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!

[chakrihacker]

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.