Validate state tree instances instead of snapshots in the SnapshotProcessor.is override

[airhorns]

What does this PR do and why?

This fixes a bug where within .is on models with snapshot processors, we validated against the model snapshot, instead of the model node when given a node. This means that two different models with compatible snapshots would not .is instances of each other normally, but once a snapshot processor is added, they would. This adds a test capturing this case that I verified failed on master and passes now.

The PR that introduced this behaviour of validating the snapshot was introduced here: #1495, and I think was actually targeting a different use case -- passing snapshots to .is, not passing instances. The behaviour that PR added was to validate against the processed version of a snapshot if passed a snapshot, which this PR maintains. But, if passed an instance, that PR elected to snapshot it as well, which I don't think is necessary, and breaks the substitutability of snapshot processed instances with their unwrapped counterparts.

Steps to validate locally

Tests should capture this mostly, but, I am not super aware of the coverage of this thing, so looking for any and all feedback on if this approach makes sense.

[coolsoftwaretyler]

Thanks for the PR, test, and description @airhorns!

This strikes me as a potentially breaking change, so I'd like to take it a little slowly. The code looks good to me, but I want to leave this open for other comments, and I want to think on it before we merge.

The tests illustrate the change quite well, but is there any chance you could describe a real world scenario that this would improve? I do not use the .is explicitly that much.

[airhorns]

That makes sense, let us take our time! I feel like the strongest justification in my mind is aligning the behaviour of .is on snapshot-processed instances with that of plain old models. It makes sense to me that an empty snapshot processor shouldn't change the behavior of anything about a model, but it currently does, which violates Liskov substitutability and is just in general surprising.

I found this because we added a snapshot processor to one of our models in Gadget and then a whole bunch of unions which included that model started selecting it for a type, because all it's properties were optional, and any snapshot thusly matched. Any instance does not though. A complicated example but shows that the substititibility thing is valuable!

[coolsoftwaretyler]

Thanks, @airhorns! That's really helpful context. I'll bring this up at the maintainer meeting tomorrow.

I've also gone ahead and published a preview release with this change. You can try it out with version 6.0.1-pre.1, and I'll see if I can solicit other folks to do the same (I'll try it out with my app/test suite at least): https://www.npmjs.com/package/mobx-state-tree/v/6.0.1-pre.1

[coolsoftwaretyler]

Hey @airhorns - I'm talking with Jason later today on the maintainer meeting, so I'll bring this up there. But for posterity/open discussion, I'm a little concerned this will break some existing expectations in our type checking: https://github.com/mobxjs/mobx-state-tree/blob/master/src/core/type/type-checker.ts#L80

It looks like our TypeChecking error strings want to flag compatible snapshots:

function toErrorString(error: IValidationError): string {
  const { value } = error
  const type = error.context[error.context.length - 1].type!
  const fullPath = error.context
    .map(({ path }) => path)
    .filter((path) => path.length > 0)
    .join("/")

  const pathPrefix = fullPath.length > 0 ? `at path "/${fullPath}" ` : ``

  const currentTypename = isStateTreeNode(value)
    ? `value of type ${getStateTreeNode(value).type.name}:`
    : isPrimitive(value)
    ? "value"
    : "snapshot"
  const isSnapshotCompatible =
    type && isStateTreeNode(value) && type.is(getStateTreeNode(value).snapshot)

  return (
    `${pathPrefix}${currentTypename} ${prettyPrintValue(value)} is not assignable ${
      type ? `to type: \`${type.name}\`` : ``
    }` +
    (error.message ? ` (${error.message})` : "") +
    (type
      ? isPrimitiveType(type) || isPrimitive(value)
        ? `.`
        : `, expected an instance of \`${(type as IAnyType).name}\` or a snapshot like \`${(
            type as IAnyType
          ).describe()}\` instead.` +
          (isSnapshotCompatible
            ? " (Note that a snapshot of the provided value is compatible with the targeted type)"
            : "")
      : `.`)
  )
}

This is a minor concern for me, because it's not on the critical path for most people (we don't type check in production anyway). But I'm curious if you think the original error behavior is incorrect or unhelpful, or if there's a way this change might preserve that behavior.

[coolsoftwaretyler]

Hey folks, I haven't heard much pushback at all and I'm feeling pretty solid on this one.

@thegedge - would you mind writing up what you said to me about the snapshot in distinction at the maintainer meeting?

Once you do that, I'll merge. I'm considering marking this as a breaking change, but with a very very straightforward migration path (none required really), just a clearer definition of what we mean when we talk about is equality.

[thegedge]

Oh yeah, my bad for forgetting to copy what we said here.

So as @coolsoftwaretyler said, this is indeed a breaking change, but it's also moving us towards what I would say is the correct contract for is. For reference:

mobx-state-tree/src/core/type/type.ts

Lines 98 to 104 in e69dc4c

	/**
	* Checks if a given snapshot / instance is of the given type.
	*
	* @param thing Snapshot or instance to be checked.
	* @returns true if the value is of the current type, false otherwise.
	*/
	is(thing: any): thing is C | this["Type"]

With regards to the type guard on that function:

• C is a SnapshotIn
• this["Type"] is (essentially) an Instance

Note the lack of a SnapshotOut, which is what you get from getSnapshot. This is definitely what we want, because we want to basically say "hey, you can take this thing and put it in a map or instantiate a type with it".

So we're thinking this is the right move, but perhaps we just document this even more clearly in the JSDoc's for is (maybe scan existing docs to see if we talk about is anywhere and update those too, or perhaps write some new ones).

[coolsoftwaretyler]

Thank you!

I'll incorporate some of that into the docs, and get this merged for a breaking release because I think this clarification is correct, but it's conceivable that some users have different assumptions, so I want to give them a semantic way to protect against the clearer guidelines.

I'll sequence this so that #2189 comes in as a patch, and then we release this as a new major (with a very straightforward migration path, haha).

Should be ready to go sometime early/mid next week. For major versions, I like to keep pre-releases available for a while to solicit feedback, but in this case we can just run it a few weeks I think.

[coolsoftwaretyler]

I just released v6.0.1, so I'm feeling good about merging this and putting together a preview for v7 early this week. Should ge around to it later today or sometime Monday/Tuesday.

[coolsoftwaretyler]

Rebased. I'll add those notes, merge this in, and then get a new major preview out.

[coolsoftwaretyler]

Merged and published as a preview version here: https://www.npmjs.com/package/mobx-state-tree/v/7.0.0-pre.1

I'll make a release post and tweet about it 'n stuff.

It's a very easy migration from v6 to v7 for most folks, I'd wager. I'll wait about a month before we mark it as latest.