search

mobxjs / mobx-state-tree

Full-featured reactive state management without the boilerplate
https://mobx-state-tree.js.org/
MIT License
6.9k stars 639 forks source link

feat: pass the node to postProcess snapshot transformers #2116

Closed airhorns closed 7 months ago

airhorns commented 8 months ago

What does this PR do and why?

When transforming the snapshot for a node, it would be quite handy to get access to the node the snapshot was generated for, so you can traverse the tree to get at data you want to put in the snapshot. Stored properties of that node are already given to you in the hook in the snapshot, but computeds, data from parents, tree environment data, or volatiles aren't accessible for use in snapshot transformers unless the node is passed in.

This brings back half of #2065 which was reverted as the preProcess changes weren't a satisfactory API. I think that decision is wise, but this PR targets postProcess only. I think it should always be possible to pass the node in to postProcess, as we're never in the middle of constructing a node.

My concrete use case for this is caching the output of some expensive computed views in the snapshot itself. I don't think that needs to be a core MST feature, but for the brave among us who want to venture into that territory, I think it'd be great if MST made it possible.

Steps to validate locally

Added some tests, kept the existing test suite passing! Annoyingly, if we do want to pass the node in, the node has to exist whenever we go to snapshot, and that means createObservableInstanceIfNeeded has to be called for nodes with a postProcessor a bit earlier than it used to be. I think that is ok as not that many nodes have postProcessors.

cc @BrianHung @coolsoftwaretyler

coolsoftwaretyler commented 8 months ago

Hey @airhorns - thank you for this. I think this is pretty interesting. I really appreciate the context you provided, tests, and documentation changes.

I've got a busy couple of days coming up, but I can look at this in depth over the weekend.

While browsing the diff, I saw this in the hooks documentation:

Note: pre and post processing are just meant to convert your data into types that are more acceptable to MST. Typically it should be the case that `postProcess(preProcess(snapshot)) === snapshot. If that isn't the case, consider whether you shouldn't be using a dedicated a view instead to normalize your snapshot to some other format you need.

Would this PR break that documented intention? Or have I misunderstood what this code would do/what that section is saying?

airhorns commented 8 months ago

I think that this capability would indeed let you violate that intention. In general, I think that doc is correct, and the typical use of snapshot processors is to be pure functions that are the inverse of each other. For my use case (caching views in snapshots), the snapshot itself now has extra information in it for sure such that they aren't the inverse of one another, but, that information is recoverable just by running the view. So IMO my use case still complies with the spirit of that note, which I think is "don't use random state in snapshots that you might not have next time".

coolsoftwaretyler commented 8 months ago

Thanks! That's pretty convincing in my opinion. I'll take a closer look this weekend!

coolsoftwaretyler commented 7 months ago

Thanks for the follow-up @airhorns! I left one of my comments open for @chakrihacker or anyone else who might come check out the PR. I'm feeling good about this. Hoping to get a second maintainer review on it soon.

Have a great weekend!

chakravarthi-bb commented 7 months ago

Hey @airhorns PR is really good. I want to do some local testing. I will do it in the evening and update it.

coolsoftwaretyler commented 7 months ago

Thanks for the review @chakrihacker. Looks like @airhorns has implemented your feedback, so I'm going to merge this and get started on a pre release.