Skip to content

Commit

Permalink
Validate state tree instances instead of snapshots in the `SnapshotPr…
Browse files Browse the repository at this point in the history
…ocessor.is` override (#2182)

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

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 failing test capturing this case.

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.

* docs: bump JSdocon snapshotProcessor is

* docs: one more link for snapshotProcessor

* chore: version bump to 7.0.0-pre.1

---------

Co-authored-by: Tyler Scott Williams <[email protected]>
  • Loading branch information
airhorns and coolsoftwaretyler authored Jul 12, 2024
1 parent 9c199a5 commit 0ef2ba2
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 2 deletions.
22 changes: 22 additions & 0 deletions __tests__/core/snapshotProcessor.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -642,6 +642,28 @@ describe("snapshotProcessor", () => {
expect(ProcessedModel.is(Model)).toBe(false)
})

test(".is checks instances against the underlying type", () => {
const ModelA = types.model({ x: types.number })
const ModelB = types.model({ x: types.number })
const modelA = ModelA.create({ x: 1 })
const modelB = ModelB.create({ x: 2 })

// despite having the same snapshot type, .is is false for the instance of one against the other because they are not the same type
expect(ModelA.is(modelA)).toBe(true)
expect(ModelB.is(modelA)).toBe(false)
expect(ModelA.is(modelB)).toBe(false)
expect(ModelB.is(modelB)).toBe(true)

const ProcessedModel = types.snapshotProcessor(ModelA, {})

const processedModel = ProcessedModel.create({ x: 3 })
expect(ProcessedModel.is(processedModel)).toBe(true)
expect(ModelA.is(processedModel)).toBe(true)
expect(ModelB.is(processedModel)).toBe(false)
expect(ProcessedModel.is(modelA)).toBe(true)
expect(ProcessedModel.is(modelB)).toBe(false)
})

describe("1776 - reconciliation in an array", () => {
test("model with transformed property is reconciled", () => {
const SP = types.snapshotProcessor(
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "mobx-state-tree",
"version": "6.0.1",
"version": "7.0.0-pre.1",
"description": "Opinionated, transactional, MobX powered state container",
"main": "dist/mobx-state-tree.js",
"umd:main": "dist/mobx-state-tree.umd.js",
Expand Down
19 changes: 18 additions & 1 deletion src/types/utility-types/snapshotProcessor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -149,11 +149,28 @@ class SnapshotProcessor<IT extends IAnyType, CustomC, CustomS> extends BaseType<
return this._subtype
}

/**
* MST considers a given value to "be" of a subtype is the value is either:
*
* 1. And instance of the subtype
* 2. A valid snapshot *in* of the subtype
*
* Before v7, we used to also consider processed models (as in, SnapshotOut values of this).
* This is no longer the case, and is more in line with our overall "is" philosophy, which you can
* see in `src/core/type/type.ts:104` (assuming lines don't change too much).
*
* For additonal commentary, see discussion in https://github.com/mobxjs/mobx-state-tree/pull/2182
*
* The `is` function specifically checks for `SnapshotIn` or `Instance` of a given type.
*
* @param thing
* @returns
*/
is(thing: any): thing is any {
const value = isType(thing)
? this._subtype
: isStateTreeNode(thing)
? getSnapshot(thing, false)
? thing
: this.preProcessSnapshotSafe(thing)
if (value === $preProcessorFailed) {
return false
Expand Down

0 comments on commit 0ef2ba2

Please sign in to comment.