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

Unexpected patches for Dates with applySnapshot #2215

Open
3 tasks done
thegedge opened this issue Sep 16, 2024 · 3 comments
Open
3 tasks done

Unexpected patches for Dates with applySnapshot #2215

thegedge opened this issue Sep 16, 2024 · 3 comments
Labels
enhancement Possible enhancement help/PR welcome Help/Pull request from contributors to fix the issue is welcome level: intermediate

Comments

@thegedge
Copy link
Collaborator

Bug report

  • I've checked documentation and searched for existing issues and discussions
  • I've made sure my project is based on the latest MST version
  • Fork this code sandbox or another minimal reproduction.

Sandbox link or minimal reproduction code

https://codesandbox.io/p/sandbox/mst-autorun-example-forked-7ds6cs?file=%2Findex.ts%3A1%2C1-35%2C1&workspaceId=84a0645a-1491-4074-88e5-f44a8818ef46

Describe the expected behavior

When running applySnapshot without changing the value for a types.Date, I expect no patches to be emitted.

Describe the observed behavior

Patches are emitted even though the values haven't changed.

CleanShot 2024-09-16 at 16 43 51@2x

Other notes

I can guess at the implementation here and why the patch is being emitted: new Date(123) != new Date(123).

What I'm wondering is how far we should go to minimize the patches 🤔

Do we close this as "working as intended"? If not, should we avoid emitting a patch for a types.frozen with the value { value: "a" } and having a snapshot applied with the same value (but a difference instance)?

@coolsoftwaretyler
Copy link
Collaborator

Looks related to #2080

We have that marked as a breaking change and I haven't released v7 yet so maybe we consider this and bundle it there...

@thegedge
Copy link
Collaborator Author

Ah, good catch @coolsoftwaretyler. I was focused on looking for generic snapshot issues, not something specific to dates.

Separate from that, we can perhaps leave this issue to discuss if we want the equality to go any deeper. Could users somehow hook into types.frozen with a "comparer" to avoid generating patches when nothing is changing?

@coolsoftwaretyler
Copy link
Collaborator

Yeah I like that. We could maybe even avoid a breaking change here by writing something like that. If we provide custom comparators for certain data types (or all perhaps?) - we could preserve the default behavior as-is, and allow users to opt-in to another behavior.

I'm going to mark this as an enhancement for now.

@coolsoftwaretyler coolsoftwaretyler added enhancement Possible enhancement help/PR welcome Help/Pull request from contributors to fix the issue is welcome level: intermediate labels Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Possible enhancement help/PR welcome Help/Pull request from contributors to fix the issue is welcome level: intermediate
Projects
None yet
Development

No branches or pull requests

2 participants