Fix snapshotProcessor.is

[hovsater]

No description provided.

[mweststrate]

The change itself looks fine, but can you explain why the existing test needed updating? It now changes it's behavior and no longer converts a string to a number.

Also, can you introduce a new test that hit's and confirms the change in this PR?

[hovsater]

@mweststrate thank you for the prompt reply, appreciate it. 🙂

The existing test was updated to reflect the change I made in this pull request. Without the change, the test will fail. In addition, the test is still valid for the behavior it was introduced by so I don't think an additional test is needed here.

[mweststrate]

Ok, then I don't get your change. The test is a valid case: where a string x was converted to a number x, and you changed it in totally something else (instead of changing a type, changing an attribute name, which is a valid case as well, but a different one). If that original test failed with your change, it means that you are introducing a breaking change?

Your original issue with the code sandbox had a nice test case, where a model instance wasn't properly recognized as being assignable to a snapshot type.

So what I would expect to see: the current test to be unchanged (or an explanation why that test was wrong all along). And a new test that demonstrates that model instances are now indeed correctly recognized as instances of that snapshot type.

Hope that makes sense!

[hovsater]

Let me try to explain a bit more clearly.

The intent of the original test case was to check that is on the snapshotProcessor type clearly identified instances of itself and unprocessed objects correctly. The fact that we converted a string representing a number to a number was just an implementation detail. It could have been something else entirely.

The test passed but it was accidental.

In the following expectation

expect(ProcessedModel.is(processedModel)).toBe(true)

we're passing the model instance to .is. This instance was wrongly passed through the preProcessor step to finally emit { x: 1 } again. It still made the test pass because x exists on the model instance (x is a number in this case).

So in the step:

{ ...sn, x: Number(sn.x) }

sn isn't a plain object but a model instance, and x isn't a string but a number.

---

I changed the implementation of the preProcessor step do something that's not idempotent, i.e., we cannot run the model instance through the preProcessor without first running it through the postProcessor. Now, the test fail (correctly so) because the original implementation was passing the model instance (already pre-processed) to the preProcessor step again.

The implementation was then changed to check if thing isStateTreeNode and if it is, grab a snapshot of itself instead of running it through the preProcessor step.

It should not be a breaking change since the only difference is using getSnapshot instead of the preProcessor for state tree nodes.

Hopefully this made it somewhat more clear. 🙂

[hovsater]

@mweststrate did that clear things up or do you still want me to introduce a separate test case? 🙂

[mweststrate]

Nope, thanks for the explanation and apologies for the late follow up! If you need more changes like this in the future and are interested in maintainer rights, let me know :) It allows you to branch directly on this repo and that is a much more convenient workflow.

I'll try to release this today :)

[mweststrate]

Released in 3.16.0