Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add failing test for missing snapshot processor node argument #2183

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

airhorns
Copy link
Contributor

@airhorns airhorns commented Jun 5, 2024

What does this PR do and why?

This adds a failing test for a bug introduced in #2116 . There, we started passing the node down to snapshot post processors to allow them to do fancy stuff, but, this mechanism is broken for deeply nested types that are lazily instantiated. We only instantiate the children of an array type when it is accessed or snapshotted for the first time (as best I can tell), and when this happens, the node.storedValue property is null and so the snapshot processor doesn't get the node.

The fix (I think) is to ensure that deeply nested nodes with snapshot post processors are eagerly created before we start snapshotting, but I am struggling to get that to work. Any insight would be appreciated!

Relevant past issues and PRs:

@coolsoftwaretyler
Copy link
Collaborator

This is a great place to start! I will put some thought on it shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants