Mobx-state-tree: preProcessSnapshot executed twice

[farwayer]

First for typecheck second for instantiating. Both for create and applySnapshot. This is problem because transforming can be really slow.
https://github.com/mobxjs/mobx-state-tree/blob/89224d3faecaa2883486de745a4f3a7dae06a296/src/types/type.ts#L49-L52
https://github.com/mobxjs/mobx-state-tree/blob/fd0b89095a1e70905cc7d7b0ba2a36808d602a16/src/types/complex-types/object.ts#L351-L352
https://github.com/mobxjs/mobx-state-tree/blob/9bb7295c7be320cbc56f74901754674a4eff18c2/src/types/complex-types/object.ts#L238-L247

[mattiamanzati]

Uhm, is really that slow? What does it perform? Have you considered memoizing the preprocess function? :)

[farwayer]

Pre-process function can contain sorting for example and processing can be slow for large item count.
Another problem double-executing can introduce logic bugs.

const Model = types.model({
  count: types.refinement(types.number, value => value <= 10),
})
.preProcessSnapshot(snapshot => {
  let {count=0} = snapshot;
  count *= 10;
  return {count};
});

const obj = Model.create();
applySnapshot(obj, {count: 1});

Error: [mobx-state-tree] Error while converting `{"count":10}` to `AnonymousModel`:
at path "/count" value `100` is not assignable to type: `number` (Value does not respect the refinement predicate).

count should be 10 in this case but it will be 100

[mattiamanzati]

Uhm, about the second one I think that maybe you are accidentally modifying the incoming snapshot (we should indeed freeze it in dev mode).
Please try this instead:

.preProcessSnapshot(snapshot => {
  let {count=0} = snapshot;
  return {count: count*10};
});

The readme warns about that, as said maybe we should freeze the given snapshot :)

[farwayer]

There is no snapshot modification in my code

[mweststrate]

ok, maybe we should explicitly mention in README that preProcessing should
idempotent, not just immutable :)

Op zo 3 sep. 2017 om 21:34 schreef farwayer [e-mail]:

There is no snapshot modification in my code

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/mobxjs/mobx-state-tree/issues/348#issuecomment-326826302,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABvGhDhafR67eShugm54M4BG4WVpJNS6ks5sev9RgaJpZM4PJnEL
.

[farwayer]

I pretty sure more correct will be to transform data only once. From point of performance and logic.
One example: if server sends percents for some value and I want to keep it as fraction then it will be hard to make preProcessSnapshot idempotent.

[mweststrate]

Yeah that was my initial hunch as well. But it makes typechecking stateful. Take for example C = types.union(A, B), and A has a preProcessor and you give it a snapshot for B. The typechecker will first verify if your snapshot is a valid A (including transforming the snapshot). So the transformation should have no lasting effects, otherwise the ultimately created B would be affected. Similarly, you could have an array(A), before we know a snapshot for that array is valid (and instantiate it), we will first check the entire snapshot whether it only contains valid A's.

In other words, avoiding the double transformation requires maintaining a cache of arbitrary depth and breath, caching any involved preprocessor on individual nodes. That is not an unsolvable problem, but definitely not trivial either.

So I think for now the best work around on performance critical pieces is to manually preprocess your data before sending it to MST.

[le0nik]

This is a really big problem in my opinion. You have to handle snapshot being both preprocessed and not preprocessed. For example:

const MyStore = types
.model('MyStore', {
  date: types.Date,
})
.preProcessSnapshot(snapshot => snapshot && ({
  date: fecha.parse(snapshot.date, 'YYYY-MM-DD'), // if we know that `date` is always in BE format
})
.actions(() => ({
  postProcessSnapshot: snapshot => ({
    date: fecha.format(snapshot.date, 'YYYY-MM-DD')
  })
});

This will actually throw in preProcessSnapshot because sometimes snapshot in it will contain date before preprocessing and it will be a string but sometimes it's going to be already preprocessed and be an instance of Date, so we'll have to add checks like this in preProcessSnapshot:

date: isString(snapshot.date) ? fecha.parse(snapshot.date, 'YYYY-MM-DD') : snapshot.date

It's very unexpected.

If it has to call preProcessSnapshot twice in a row passing result of the first call to the 2nd call it should at least call postProcessSnapshot before the 2nd call to preProcessSnapshot.

[mweststrate]

I understand that it something you need to keep in mind when using this hook, which is annoying. But as explained above it cannot be prevented inhome, as the snapshot needs to be transformed before MST is able to determine the type of a thing, for example when using unions. So this means that the snapshot pre processor of several types might need to be called before MST makes up it's mind and picks the best matching type.

Hence a won't-fix until someone has a super smart idea for this :)

[le0nik]

@mweststrate why not at least run postProcessSnapshot before the 2nd call to preProcessSnapshot to avoid unnecessary checks in preProcessSnapshot?

[pixelkritzel]

Hence a won't-fix until someone has a super smart idea for this :)

@mweststrate I might have an idea. You wrote this library immer.js and obviously you use something similar in MST to guarantee immutability. Wouldn't it be possible to utilise this for preProcessSnapshot?

[mweststrate]

I see how this is an encouragement, but that doesn't necessary make it an idea ;-). Feel free to make your idea more tangible, but please do so in a new issue as this one was closed almost 2 years ago :)