Skip to content
New issue

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

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

Already on GitHub? # to your account

feat: deepCurrent #1092

Closed
wants to merge 1 commit into from
Closed

Conversation

lveillard
Copy link

Related to this issue: #1091

This is just a draft, if it goes validated I can test it, add some tests and apply any changes you might ask for!

@mweststrate
Copy link
Collaborator

Thanks for the effort of creating this PR! I think this might actually be a bug in the current current (lol) implementation, as it is supposed to be able to handle recursion already by the looks of it: https://github.com/immerjs/immer/blob/main/src/core/current.ts#L33

From a quick scan your implementation seems also to be exactly the same as current? Would you mind first setting up a unit test showing the problem in the existing current implementation?

@lveillard
Copy link
Author

Thanks for the effort of creating this PR! I think this might actually be a bug in the current current (lol) implementation, as it is supposed to be able to handle recursion already by the looks of it: https://github.com/immerjs/immer/blob/main/src/core/current.ts#L33

From a quick scan your implementation seems also to be exactly the same as current? Would you mind first setting up a unit test showing the problem in the existing current implementation?

True! Somehow deepCurrent works for my use case and current does not 🤷‍♂️. Will try to find some time to build that test triggering the issue, I guess is related to using symbols with objects as values.

@mweststrate
Copy link
Collaborator

Closing in favour of #1105, that fixes the underlying issue (I think). Thanks for reporting and taking the effort for this PR though!

@mweststrate mweststrate closed this Mar 9, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants