feat: pass the node to postProcess snapshot transformers #2116
Conversation
|
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:
Would this PR break that documented intention? Or have I misunderstood what this code would do/what that section is saying? |
|
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". |
|
Thanks! That's pretty convincing in my opinion. I'll take a closer look this weekend! |
There was a problem hiding this comment.
@airhorns - thanks again for all of this. I am pretty happy to approve it, much as I was with #2065 - although I moved a little too quickly on that one and we found some issues with it.
I really appreciate the work you did to find the balance here, and resolve the specific node construction issues brought up in the first attempt.
It's clear to me that the community wants this since it's the second PR to this effect we've received. I'm leaning towards merging it in, especially considering the updated tests and docs.
I have two remaining concerns:
- This does change some of the existing documentation and initial intentions of MST. I think the change is subtle, but I don't want to make a unilateral decision on it since we'll be changing a longstanding library, and I don't want to subvert expectations too quickly.
- Truthfully, since I had to revert add parent to preProcessor and postProcessor #2065 - I am maybe a little reluctant to go ahead and merge a related PR on my own.
@chakrihacker - would you mind taking a look at this PR, along with my existing review? If you're satisfied with it, I'd love to merge it in. If you have any objections, I'd like to see if we can find a path forward. I think @airhorns did a great job with this and I'd love to find a way to make it work.
Thanks for everyone's time! Really appreciate it.
| | `preProcess(inputSnapshot)` | Transform a snapshot before it is applied to a node. The output snapshot must be valid for application to the wrapped type. The `preProcess` hook is passed the input snapshot, but not passed the node, as it is not done being constructed yet, and not attached to the tree. If you need to modify the node in the context of the tree, use the `afterCreate` hook. | | ||
| | `postProcess(outputSnapshot, node)` | Transform a snapshot after it has been generated from a node. The transformed value will be returned by `getSnapshot`. The `postProcess` hook is passed the initial outputSnapshot, as well as the instance object the snapshot has been generated for. It is safe to access properties of the node or other nodes when post processing snapshots. | |
airhorns
force-pushed
the
post-process-snapshots-with-node
branch
from
e0c227c to
d869723
Compare
|
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! |
|
Hey @airhorns PR is really good. I want to do some local testing. I will do it in the evening and update it. |
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. Woop woop!
airhorns
force-pushed
the
post-process-snapshots-with-node
branch
from
d869723 to
1e226a5
Compare
|
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. |
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
preProcesschanges weren't a satisfactory API. I think that decision is wise, but this PR targetspostProcessonly. I think it should always be possible to pass the node in topostProcess, 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
createObservableInstanceIfNeededhas 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