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

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

Merged
merged 8 commits into from
Apr 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 62 additions & 0 deletions __performance_tests__/large-obj.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
import {measure} from "./measure"
import produce, {
setUseProxies,
setUseStrictShallowCopy,
enableAllPlugins
} from "../dist/immer.cjs.production.min.js"

enableAllPlugins()

console.log("\n# large-obj - mutate large object\n")

const MAX = 50

const baseState = Object.fromEntries(
Array(10000)
.fill(0)
.map((_, i) => [i, i])
)

measure("immer (proxy) - with setUseStrictShallowCopy", () => {
setUseStrictShallowCopy(true)
setUseProxies(true)

for (let i = 0; i < MAX; i++) {
produce(baseState, draft => {
draft[5000]++
})
}
})

measure("immer (proxy) - without setUseStrictShallowCopy", () => {
setUseStrictShallowCopy(false)
setUseProxies(true)

for (let i = 0; i < MAX; i++) {
produce(baseState, draft => {
draft[5000]++
})
}
})

measure("immer (es5) - with setUseStrictShallowCopy", () => {
setUseStrictShallowCopy(true)
setUseProxies(false)

for (let i = 0; i < MAX; i++) {
produce(baseState, draft => {
draft[5000]++
})
}
})

measure("immer (es5) - without setUseStrictShallowCopy", () => {
setUseStrictShallowCopy(false)
setUseProxies(false)

for (let i = 0; i < MAX; i++) {
produce(baseState, draft => {
draft[5000]++
})
}
})
632 changes: 632 additions & 0 deletions __tests__/__prod_snapshots__/base.js.snap

Large diffs are not rendered by default.

888 changes: 760 additions & 128 deletions __tests__/__snapshots__/base.js.snap

Large diffs are not rendered by default.

83 changes: 58 additions & 25 deletions __tests__/base.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import {
} from "../src/immer"
import {each, shallowCopy, DRAFT_STATE} from "../src/internal"
import deepFreeze from "deep-freeze"
import cloneDeep from "lodash.clonedeep"
import * as lodash from "lodash"

jest.setTimeout(1000)
Expand All @@ -22,21 +21,39 @@ test("immer should have no dependencies", () => {
expect(require("../package.json").dependencies).toBeUndefined()
})

runBaseTest("proxy (no freeze)", true, false)
runBaseTest("proxy (autofreeze)", true, true)
runBaseTest("proxy (patch listener)", true, false, true)
runBaseTest("proxy (autofreeze)(patch listener)", true, true, true)

runBaseTest("es5 (no freeze)", false, false)
runBaseTest("es5 (autofreeze)", false, true)
runBaseTest("es5 (patch listener)", false, false, true)
runBaseTest("es5 (autofreeze)(patch listener)", false, true, true)
for (const useProxies of [true, false]) {
for (const autoFreeze of [true, false]) {
for (const useStrictShallowCopy of [true, false]) {
for (const useListener of [true, false]) {
const name = `${useProxies ? "proxy" : "es5"}:${
autoFreeze ? "auto-freeze=true" : "auto-freeze=false"
}:${
useStrictShallowCopy ? "shallow-copy=true" : "shallow-copy=false"
}:${useListener ? "use-listener=true" : "use-listener=false"}`
runBaseTest(
name,
useProxies,
autoFreeze,
useStrictShallowCopy,
useListener
)
}
}
}
}

function runBaseTest(name, useProxies, autoFreeze, useListener) {
function runBaseTest(
name,
useProxies,
autoFreeze,
useStrictShallowCopy,
useListener
) {
const listener = useListener ? function() {} : undefined
const {produce, produceWithPatches} = createPatchedImmer({
useProxies,
autoFreeze
autoFreeze,
useStrictShallowCopy
})

// When `useListener` is true, append a function to the arguments of every
Expand Down Expand Up @@ -904,10 +921,16 @@ function runBaseTest(name, useProxies, autoFreeze, useListener) {
expect(s[test]).toBeTruthy()
s.foo = true
})
expect(nextState).toEqual({
[test]: true,
foo: true
})
if (useStrictShallowCopy) {
expect(nextState).toEqual({
[test]: true,
foo: true
})
} else {
expect(nextState).toEqual({
foo: true
})
}
})

if (!global.USES_BUILD)
Expand All @@ -923,16 +946,26 @@ function runBaseTest(name, useProxies, autoFreeze, useListener) {
value: 1,
enumerable: false
})
// Non-enumerable primitive property that won't modified.
Object.defineProperty(baseState, "baz", {
value: 1,
enumerable: false
})

// When using Proxy, even non-enumerable keys will be copied if it's changed.
const canReferNonEnumerableProperty = useProxies || useStrictShallowCopy
const nextState = produce(baseState, s => {
expect(s.foo).toBeTruthy()
expect(isEnumerable(s, "foo")).toBeFalsy()
s.bar++
expect(isEnumerable(s, "foo")).toBeFalsy()
s.foo.a++
expect(isEnumerable(s, "foo")).toBeFalsy()
})
expect(nextState.foo).toBeTruthy()
expect(isEnumerable(nextState, "foo")).toBeFalsy()
if (canReferNonEnumerableProperty) expect(s.foo).toBeTruthy()
if (useStrictShallowCopy) expect(isEnumerable(s, "foo")).toBeFalsy()
if (canReferNonEnumerableProperty) s.bar++
if (useStrictShallowCopy) expect(isEnumerable(s, "foo")).toBeFalsy()
if (canReferNonEnumerableProperty) s.foo.a++
if (useStrictShallowCopy) expect(isEnumerable(s, "foo")).toBeFalsy()
})
if (canReferNonEnumerableProperty) expect(nextState.foo).toBeTruthy()
if (useStrictShallowCopy)
expect(isEnumerable(nextState, "foo")).toBeFalsy()
if (useStrictShallowCopy) expect(nextState.baz).toBeTruthy()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

})

it("can work with own computed props", () => {
Expand Down
39 changes: 39 additions & 0 deletions __tests__/not-strict-copy.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import produce, {enableAllPlugins, setUseStrictShallowCopy} from "../src/immer"

enableAllPlugins()

describe("setUseStrictShallowCopy(true)", () => {
test("keep descriptors", () => {
setUseStrictShallowCopy(true)

const base: Record<string, unknown> = {}
Object.defineProperty(base, "foo", {
value: "foo",
writable: false,
configurable: false
})
const copy = produce(base, (draft: any) => {
draft.bar = "bar"
})
expect(Object.getOwnPropertyDescriptor(copy, "foo")).toStrictEqual(
Object.getOwnPropertyDescriptor(base, "foo")
)
})
})

describe("setUseStrictShallowCopy(false)", () => {
test("ignore descriptors", () => {
setUseStrictShallowCopy(false)

const base: Record<string, unknown> = {}
Object.defineProperty(base, "foo", {
value: "foo",
writable: false,
configurable: false
})
const copy = produce(base, (draft: any) => {
draft.bar = "bar"
})
expect(Object.getOwnPropertyDescriptor(copy, "foo")).toBeUndefined()
})
})
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
"sideEffects": false,
"scripts": {
"test": "jest && yarn test:build && yarn test:flow",
"test:perf": "cd __performance_tests__ && babel-node add-data.js && babel-node todo.js && babel-node incremental.js",
"test:perf": "cd __performance_tests__ && babel-node add-data.js && babel-node todo.js && babel-node incremental.js && babel-node large-obj.js",
"test:flow": "yarn flow check __tests__/flow",
"test:build": "yarn build && NODE_ENV='production' yarn jest --config jest.config.build.js",
"watch": "jest --watch",
Expand Down
12 changes: 8 additions & 4 deletions src/core/current.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,14 @@ function currentImpl(value: any): any {
return state.base_
// Optimization: avoid generating new drafts during copying
state.finalized_ = true
copy = copyHelper(value, archType)
copy = copyHelper(
value,
archType,
state.scope_.immer_.useStrictShallowCopy_
)
state.finalized_ = false
} else {
copy = copyHelper(value, archType)
copy = copyHelper(value, archType, true)
}

each(copy, (key, childValue) => {
Expand All @@ -47,7 +51,7 @@ function currentImpl(value: any): any {
return archType === Archtype.Set ? new Set(copy) : copy
}

function copyHelper(value: any, archType: number): any {
function copyHelper(value: any, archType: number, strict: boolean): any {
// creates a shallow copy, even if it is a map or set
switch (archType) {
case Archtype.Map:
Expand All @@ -56,5 +60,5 @@ function copyHelper(value: any, archType: number): any {
// Set will be cloned as array temporarily, so that we can replace individual items
return Array.from(value)
}
return shallowCopy(value)
return shallowCopy(value, strict)
}
5 changes: 4 additions & 1 deletion src/core/finalize.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,10 @@ function finalize(rootScope: ImmerScope, value: any, path?: PatchPath) {
const result =
// For ES5, create a good copy from the draft first, with added keys and without deleted keys.
state.type_ === ProxyType.ES5Object || state.type_ === ProxyType.ES5Array
? (state.copy_ = shallowCopy(state.draft_))
? (state.copy_ = shallowCopy(
state.draft_,
rootScope.immer_.useStrictShallowCopy_
))
: state.copy_
// Finalize all children of the copy
// For sets we clone before iterating, otherwise we can get in endless loop due to modifying during iteration, see #628
Expand Down
19 changes: 18 additions & 1 deletion src/core/immerClass.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,19 @@ export class Immer implements ProducersFns {

autoFreeze_: boolean = true

constructor(config?: {useProxies?: boolean; autoFreeze?: boolean}) {
useStrictShallowCopy_: boolean = false

constructor(config?: {
useProxies?: boolean
autoFreeze?: boolean
useStrictShallowCopy?: boolean
}) {
if (typeof config?.useProxies === "boolean")
this.setUseProxies(config!.useProxies)
if (typeof config?.autoFreeze === "boolean")
this.setAutoFreeze(config!.autoFreeze)
if (typeof config?.useStrictShallowCopy === "boolean")
this.setUseStrictShallowCopy(config!.useStrictShallowCopy)
}

/**
Expand Down Expand Up @@ -195,6 +203,15 @@ export class Immer implements ProducersFns {
this.useProxies_ = value
}

/**
* Pass true to enable strict shallow copy.
*
* By default, immer does not copy the object descriptors such as getter, setter and non-enumrable properties.
*/
setUseStrictShallowCopy(value: boolean) {
this.useStrictShallowCopy_ = value
}

applyPatches<T extends Objectish>(base: T, patches: Patch[]): T {
// If a patch replaces the entire state, take that replacement as base
// before applying patches
Expand Down
12 changes: 10 additions & 2 deletions src/core/proxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
createProxy,
ProxyType
} from "../internal"
import {ImmerScope} from "./scope"

interface ProxyBaseState extends ImmerBaseState {
assigned_: {
Expand Down Expand Up @@ -273,8 +274,15 @@ export function markChanged(state: ImmerState) {
}
}

export function prepareCopy(state: {base_: any; copy_: any}) {
export function prepareCopy(state: {
base_: any
copy_: any
scope_: ImmerScope
}) {
if (!state.copy_) {
state.copy_ = shallowCopy(state.base_)
state.copy_ = shallowCopy(
state.base_,
state.scope_.immer_.useStrictShallowCopy_
)
}
}
7 changes: 7 additions & 0 deletions src/immer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,13 @@ export const setAutoFreeze = immer.setAutoFreeze.bind(immer)
*/
export const setUseProxies = immer.setUseProxies.bind(immer)

/**
* Pass true to enable strict shallow copy.
*
* By default, immer does not copy the object descriptors such as getter, setter and non-enumrable properties.
*/
export const setUseStrictShallowCopy = immer.setUseStrictShallowCopy.bind(immer)

/**
* Apply an array of Immer patches to the first argument.
*
Expand Down
7 changes: 7 additions & 0 deletions src/types/index.js.flow
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,13 @@ declare export function setAutoFreeze(autoFreeze: boolean): void
*/
declare export function setUseProxies(useProxies: boolean): void

/**
* Pass false to disable strict shallow copy.
*
* By default, immer does not copy the object descriptors such as getter, setter and non-enumrable properties.
*/
declare export function setUseStrictShallowCopy(useStrictShallowCopy: boolean): void

declare export function applyPatches<S>(state: S, patches: Patch[]): S

declare export function original<S>(value: S): S
Expand Down
13 changes: 12 additions & 1 deletion src/utils/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -162,8 +162,19 @@ export function latest(state: ImmerState): any {
}

/*#__PURE__*/
export function shallowCopy(base: any) {
export function shallowCopy(base: any, strict: boolean) {
if (Array.isArray(base)) return Array.prototype.slice.call(base)

if (!strict && isPlainObject(base)) {
const keys = Object.keys(base)
const obj: any = Object.create(Object.getPrototypeOf(base))
for (let i = 0; i < keys.length; i++) {
const key = keys[i]
obj[key] = base[key]
}
return obj
}

const descriptors = getOwnPropertyDescriptors(base)
delete descriptors[DRAFT_STATE as any]
let keys = ownKeys(descriptors)
Expand Down
1 change: 1 addition & 0 deletions website/docs/api.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ title: API overview
| `produceWithPatches` | Works the same as `produce`, but instead of just returning the produced object, it returns a tuple, consisting of `[result, patches, inversePatches]`. | [Patches](./patches.mdx) |
| `setAutoFreeze` | Enables / disables automatic freezing of the trees produces. By default enabled. | [Freezing](./freezing.mdx) |
| `setUseProxies` | Can be used to disable or force the use of `Proxy` objects. Useful when filing bug reports. | |
| `setUseStrictShallowCopy` | Can be used to enable strict shallow copy. If enable, immer copies non-enumerable properties as much as possible. | [Classes](./complex-objects.md) |

## Importing immer

Expand Down
Loading