[breaking change] implement and default to useStrictShallowCopy, omitting getters

[hrsh7th]

Description

Fixes #867

This PR improves the performance when updating the object that contains a large number of entries.

Problem

The current immer takes a long time for creating new object when the draft object has large amount of keys.

From some of the investigation, I found the cause that is getOwnPropertyDescriptors (here) .

In my computer, the ownKeys and its iteration don't take a long time but getOwnPropertyDescriptors takes time near the 10ms.

I can understand that the getOwnPropertyDescriptors is needed. Because immer should copy object's descriptors such as getter/setter/non-enumerable keys.

But some of the users doesn't need that strictness, I think.

Solution

Add the ability to opt-out of strict shallow copy implementation for performance reasons.

edited

Make the default behavior not strict shallow copy. And add the ability to opt-in to strict shallow copying.

import { useStrictShallowCopy } from 'immer';

useStrictShallowCopy(true); // opt-in strict shallow copy.

Result

The following results show the benefit of this patch.

// Iteration count:
//   50
//
// Base object: 
//   Array(10000)
//   	.fill(0)
//   	.map((_, i) => [i, i])
// 
// Test case:
//   for (let i = 0; i < MAX; i++) {
//   	produce(baseState, draft => {
//   		draft[5000]++
//   	})
//   }

# large-obj - mutate large object

immer (proxy) - with setUseStrictShallowCopy: 354ms
immer (proxy) - without setUseStrictShallowCopy: 79ms
immer (es5) - with setUseStrictShallowCopy: 1228ms
immer (es5) - without setUseStrictShallowCopy: 655ms

[netlify]

👷 Deploy request for quizzical-lovelace-dcbd6a pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	f9cdf8e

[mweststrate]

I didn't read the code yet, but could you include a rationale in the PR description, what are you updating and why is it faster, what does the API look like, in which cases will it break things, etc?

[coveralls]

Pull Request Test Coverage Report for Build 2343830328

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.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 17 of 18 (94.44%) changed or added relevant lines in 5 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.1%) to 98.433%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/core/immerClass.ts	3	4	75.0%

Totals
Change from base Build 2313164511:	-0.1%
Covered Lines:	797
Relevant Lines:	806

---

💛 - Coveralls

[hrsh7th]

Thank you for your advice.
I will update the PR description and try a more rational explanation.
It may take some time.

[gregdingle]

@hrsh7th @mweststrate this looks like a great PR to give users (like me) meaningfully better performance with large objects (>1000 keys).

Was it closed just for lack of activity?

[mweststrate]

It was missing rationale IIRC when it was closed, but now I see it is present, so reopening :) It might take some time to process though due to my own personal circumstances. Apologies upfront!

[mweststrate]

I think it makes sense to make this behavior the new default, ship it as separate major version and make the old behavior opt-in using a flag.

[hrsh7th]

Thank you for your review.

Nice to hear that this PR is an acceptable change.

I noticed that the test is failing. I will fix it.

[hrsh7th]

Could you approve the GitHub Actions?

In my environment, this branch is passed the all tests... I want to re-try GitHub Actions.

[mweststrate]

done!

[hrsh7th]

Thank you! It seems to pass the tests.
I'll do the necessary work, such as fixes that change the default behavior.

[hrsh7th]

This test is ugly, but after some consideration, I think it should be the spec.

First, I thought that non-enumerable properties should not be visible from drafts when useStrictShallowCopy is false.

However, achieving this behavior requires checking whether the referenced property is non-enumerable, which introduces new overhead.

[hrsh7th]

I'm not confident about the documentation part, but I think the code part is ready for review.

[github-actions]

🎉 This PR is included in version 10.0.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀