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

non-enumerable (immutable) methods on Map/Set #1069

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gimelg
Copy link

@gimelg gimelg commented Sep 10, 2023

when an immer object includes a Map/Set, the mutating methods are replaced with ones that prevent mutation outside of the produce function.  to prevent cluttering the console when inspecting the Map/Set, these methods are set to non-enumerable. this behavior also matches that of the original methods that are replaced.

when an immer object includes a Map/Set, the mutating methods are replaced with ones that prevent mutation outside of the procude function.  
to prevent cluttering the console when inspecting the Map/Set, these methods are set to non-enumerable. this behavior also matches that of the original methods that are replaced.
@@ -188,7 +188,12 @@ export function freeze<T>(obj: T, deep?: boolean): T
export function freeze<T>(obj: any, deep: boolean = false): T {
if (isFrozen(obj) || isDraft(obj) || !isDraftable(obj)) return obj
if (getArchtype(obj) > 1 /* Map or Set */) {
obj.set = obj.add = obj.clear = obj.delete = dontMutateFrozenCollections as any
Object.defineProperties(obj, {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good to me! As a nit optimisation, since the property map is constant, it could be lifted outside the function (usually that really doesn't matter, but with freeze potentially touching a gazillion of objects it might)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'm waiting with sending the updated PR because the current state of the repo is failing some tests, both locally and using 'Open in Gitpod' (albeit a bit differently – locally tests/produce.ts, tests/draft.ts, and tests/redux.ts suits are failing, while in Gitpod the tests/immutable.ts is failing in addition to the other three).

@coveralls
Copy link

coveralls commented Oct 2, 2024

Pull Request Test Coverage Report for Build 6139747876

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 99.793%

Totals Coverage Status
Change from base Build 4929177933: 0.0%
Covered Lines: 646
Relevant Lines: 647

💛 - Coveralls

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.

3 participants