From 2ca8339341e7c627729e0bd74a337ec379048343 Mon Sep 17 00:00:00 2001 From: Daishi Kato Date: Sun, 7 Apr 2024 20:58:53 +0900 Subject: [PATCH 1/4] chore: old ts versions to test in ci (#2489) --- .github/workflows/test-old-typescript.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/test-old-typescript.yml b/.github/workflows/test-old-typescript.yml index 47980a2c97..ac9e32b462 100644 --- a/.github/workflows/test-old-typescript.yml +++ b/.github/workflows/test-old-typescript.yml @@ -13,7 +13,8 @@ jobs: fail-fast: false matrix: typescript: - - 5.3.2 + - 5.4.4 + - 5.3.3 - 5.2.2 - 5.1.6 - 5.0.4 From 93a28f4d0b87c5d9fec4932e9ee0dcd67c1cec0a Mon Sep 17 00:00:00 2001 From: Oliver Hoff Date: Sun, 7 Apr 2024 14:06:41 +0200 Subject: [PATCH 2/4] fix(vanilla): setters do not freeze new values (#2476) Co-authored-by: Daishi Kato --- src/vanilla/utils/freezeAtom.ts | 62 ++++++++++++------- tests/react/vanilla-utils/freezeAtom.test.tsx | 44 ++++++++++--- 2 files changed, 74 insertions(+), 32 deletions(-) diff --git a/src/vanilla/utils/freezeAtom.ts b/src/vanilla/utils/freezeAtom.ts index e779c2d4aa..585274bf17 100644 --- a/src/vanilla/utils/freezeAtom.ts +++ b/src/vanilla/utils/freezeAtom.ts @@ -1,9 +1,6 @@ -import { atom } from '../../vanilla.ts' -import type { Atom } from '../../vanilla.ts' +import type { Atom, WritableAtom } from '../../vanilla.ts' -const cache1 = new WeakMap() -const memo1 = (create: () => T, dep1: object): T => - (cache1.has(dep1) ? cache1 : cache1.set(dep1, create())).get(dep1) +const frozenAtoms = new WeakSet>() const deepFreeze = (obj: unknown) => { if (typeof obj !== 'object' || obj === null) return @@ -18,25 +15,44 @@ const deepFreeze = (obj: unknown) => { export function freezeAtom>( anAtom: AtomType, -): AtomType { - return memo1(() => { - const frozenAtom = atom( - (get) => deepFreeze(get(anAtom)), - (_get, set, arg) => set(anAtom as never, arg), - ) - return frozenAtom as never - }, anAtom) +): AtomType +export function freezeAtom( + anAtom: WritableAtom, +): WritableAtom { + if (frozenAtoms.has(anAtom)) { + return anAtom + } + frozenAtoms.add(anAtom) + + const origRead = anAtom.read + anAtom.read = function (get, options) { + return deepFreeze(origRead.call(this, get, options)) + } + if ('write' in anAtom) { + const origWrite = anAtom.write + anAtom.write = function (get, set, ...args) { + return origWrite.call( + this, + get, + (...setArgs) => { + if (setArgs[0] === anAtom) { + setArgs[1] = deepFreeze(setArgs[1]) + } + + return set(...setArgs) + }, + ...args, + ) + } + } + return anAtom } export function freezeAtomCreator< - CreateAtom extends (...params: never[]) => Atom, ->(createAtom: CreateAtom): CreateAtom { - return ((...params: never[]) => { - const anAtom = createAtom(...params) - const origRead = anAtom.read - anAtom.read = function (get, options) { - return deepFreeze(origRead.call(this, get, options)) - } - return anAtom - }) as never + CreateAtom extends (...args: unknown[]) => Atom, +>(createAtom: CreateAtom): CreateAtom +export function freezeAtomCreator( + createAtom: (...args: unknown[]) => WritableAtom, +): (...args: unknown[]) => WritableAtom { + return (...args) => freezeAtom(createAtom(...args)) } diff --git a/tests/react/vanilla-utils/freezeAtom.test.tsx b/tests/react/vanilla-utils/freezeAtom.test.tsx index dfe4e7e0cb..52541827c2 100644 --- a/tests/react/vanilla-utils/freezeAtom.test.tsx +++ b/tests/react/vanilla-utils/freezeAtom.test.tsx @@ -1,43 +1,69 @@ import { StrictMode } from 'react' -import { render } from '@testing-library/react' +import { fireEvent, render } from '@testing-library/react' import { it } from 'vitest' import { useAtom } from 'jotai/react' import { atom } from 'jotai/vanilla' import { freezeAtom, freezeAtomCreator } from 'jotai/vanilla/utils' it('freezeAtom basic test', async () => { - const objAtom = atom({ count: 0 }) + const objAtom = atom({ deep: {} }, (_get, set, _ignored?) => { + set(objAtom, { deep: {} }) + }) const Component = () => { - const [obj] = useAtom(freezeAtom(objAtom)) + const [obj, setObj] = useAtom(freezeAtom(objAtom)) - return
isFrozen: {`${Object.isFrozen(obj)}`}
+ return ( + <> + +
+ isFrozen: {`${Object.isFrozen(obj) && Object.isFrozen(obj.deep)}`} +
+ + ) } - const { findByText } = render( + const { getByText, findByText } = render( , ) await findByText('isFrozen: true') + + fireEvent.click(getByText('change')) + + await findByText('isFrozen: true') }) it('freezeAtomCreator basic test', async () => { const createFrozenAtom = freezeAtomCreator(atom) - const objAtom = createFrozenAtom({ count: 0 }) + const objAtom = createFrozenAtom({ deep: {} }, (_get, set, _ignored?) => { + set(objAtom, { deep: {} }) + }) const Component = () => { - const [obj] = useAtom(objAtom) + const [obj, setObj] = useAtom(objAtom) - return
isFrozen: {`${Object.isFrozen(obj)}`}
+ return ( + <> + +
+ isFrozen: {`${Object.isFrozen(obj) && Object.isFrozen(obj.deep)}`} +
+ + ) } - const { findByText } = render( + const { getByText, findByText } = render( , ) await findByText('isFrozen: true') + + fireEvent.click(getByText('change')) + + await findByText('isFrozen: true') }) From 2abd51c466b08cd0cd91303aed127e374998cf18 Mon Sep 17 00:00:00 2001 From: Daishi Kato Date: Sun, 7 Apr 2024 21:45:24 +0900 Subject: [PATCH 3/4] deprecate freezeAtomCreator (#2490) --- docs/guides/debugging.mdx | 23 +++---- src/vanilla/utils/freezeAtom.ts | 14 +++-- tests/react/vanilla-utils/freezeAtom.test.tsx | 61 +++++++++++-------- 3 files changed, 55 insertions(+), 43 deletions(-) diff --git a/docs/guides/debugging.mdx b/docs/guides/debugging.mdx index 79f8feb0e4..1e3fa6798e 100644 --- a/docs/guides/debugging.mdx +++ b/docs/guides/debugging.mdx @@ -135,8 +135,9 @@ Which returns atoms value that is deeply freezed with `Object.freeze`. freezeAtom(anAtom): AtomType ``` -`freezeAtom` takes an existing atom and returns a new derived "frozen" atom. -The atom will be deeply frozen by `Object.freeze`. +`freezeAtom` takes an existing atom and make it "frozen". +It returns the same atom. +The atom value will be deeply frozen by `Object.freeze`. It is useful to find bugs where you unintentionally tried to change objects (states) which can lead to unexpected behavior. You may use `freezeAtom` with all atoms to prevent this situation. @@ -156,14 +157,14 @@ const objAtom = freezeAtom(atom({ count: 0 })) ### freezeAtomCreator -```js -import { atom } from 'jotai' -import { freezeAtomCreator } from 'jotai/utils' +If you need, you can define a factory for `freezeAtom`. -const createFrozenAtom = freezeAtomCreator(atom) -const objAtom = createFrozenAtom({ count: 0 }) -``` +```ts +import { freezeAtom } from 'jotai/utils' -Instead of create a frozen atom from an existing atom, -`freezeAtomCreator` takes an atom creator function and returns a new function. -You can use this not only for `atom`, but also for other `atomWith*` creators such as `atomWithReduer`. +export function freezeAtomCreator< + CreateAtom extends (...args: unknown[]) => Atom, +>(createAtom: CreateAtom): CreateAtom { + return ((...args: unknown[]) => freezeAtom(createAtom(...args))) as never +} +``` diff --git a/src/vanilla/utils/freezeAtom.ts b/src/vanilla/utils/freezeAtom.ts index 585274bf17..945a3705b2 100644 --- a/src/vanilla/utils/freezeAtom.ts +++ b/src/vanilla/utils/freezeAtom.ts @@ -16,6 +16,7 @@ const deepFreeze = (obj: unknown) => { export function freezeAtom>( anAtom: AtomType, ): AtomType + export function freezeAtom( anAtom: WritableAtom, ): WritableAtom { @@ -48,11 +49,14 @@ export function freezeAtom( return anAtom } +/** + * @deprecated Define it on users end + */ export function freezeAtomCreator< CreateAtom extends (...args: unknown[]) => Atom, ->(createAtom: CreateAtom): CreateAtom -export function freezeAtomCreator( - createAtom: (...args: unknown[]) => WritableAtom, -): (...args: unknown[]) => WritableAtom { - return (...args) => freezeAtom(createAtom(...args)) +>(createAtom: CreateAtom): CreateAtom { + console.warn( + '[DEPRECATED] freezeAtomCreator is deprecated, define it on users end', + ) + return ((...args: unknown[]) => freezeAtom(createAtom(...args))) as never } diff --git a/tests/react/vanilla-utils/freezeAtom.test.tsx b/tests/react/vanilla-utils/freezeAtom.test.tsx index 52541827c2..37f2b74d84 100644 --- a/tests/react/vanilla-utils/freezeAtom.test.tsx +++ b/tests/react/vanilla-utils/freezeAtom.test.tsx @@ -1,6 +1,6 @@ import { StrictMode } from 'react' import { fireEvent, render } from '@testing-library/react' -import { it } from 'vitest' +import { afterEach, beforeEach, describe, it, vi } from 'vitest' import { useAtom } from 'jotai/react' import { atom } from 'jotai/vanilla' import { freezeAtom, freezeAtomCreator } from 'jotai/vanilla/utils' @@ -12,7 +12,6 @@ it('freezeAtom basic test', async () => { const Component = () => { const [obj, setObj] = useAtom(freezeAtom(objAtom)) - return ( <> @@ -32,38 +31,46 @@ it('freezeAtom basic test', async () => { await findByText('isFrozen: true') fireEvent.click(getByText('change')) - await findByText('isFrozen: true') }) -it('freezeAtomCreator basic test', async () => { - const createFrozenAtom = freezeAtomCreator(atom) - const objAtom = createFrozenAtom({ deep: {} }, (_get, set, _ignored?) => { - set(objAtom, { deep: {} }) +describe('freezeAtomCreator', () => { + let savedConsoleWarn: any + beforeEach(() => { + savedConsoleWarn = console.warn + console.warn = vi.fn() + }) + afterEach(() => { + console.warn = savedConsoleWarn }) - const Component = () => { - const [obj, setObj] = useAtom(objAtom) + it('freezeAtomCreator basic test', async () => { + const createFrozenAtom = freezeAtomCreator(atom) + const objAtom = createFrozenAtom({ deep: {} }, (_get, set, _ignored?) => { + set(objAtom, { deep: {} }) + }) - return ( - <> - -
- isFrozen: {`${Object.isFrozen(obj) && Object.isFrozen(obj.deep)}`} -
- - ) - } - - const { getByText, findByText } = render( - - - , - ) + const Component = () => { + const [obj, setObj] = useAtom(objAtom) + return ( + <> + +
+ isFrozen: {`${Object.isFrozen(obj) && Object.isFrozen(obj.deep)}`} +
+ + ) + } - await findByText('isFrozen: true') + const { getByText, findByText } = render( + + + , + ) - fireEvent.click(getByText('change')) + await findByText('isFrozen: true') - await findByText('isFrozen: true') + fireEvent.click(getByText('change')) + await findByText('isFrozen: true') + }) }) From 88303acb990a7b796b0659960df3ba83b6118951 Mon Sep 17 00:00:00 2001 From: Daishi Kato Date: Sun, 7 Apr 2024 21:56:00 +0900 Subject: [PATCH 4/4] add memory leak test (#2487) --- package.json | 1 + tests/vanilla/memoryleaks.test.ts | 14 ++++++++++++++ yarn.lock | 13 +++++++++++++ 3 files changed, 28 insertions(+) create mode 100644 tests/vanilla/memoryleaks.test.ts diff --git a/package.json b/package.json index 695ad47386..86148f77c8 100644 --- a/package.json +++ b/package.json @@ -150,6 +150,7 @@ "eslint-plugin-react": "^7.34.1", "eslint-plugin-react-hooks": "^4.6.0", "eslint-plugin-vitest": "^0.4.1", + "jest-leak-detector": "^29.7.0", "jsdom": "^24.0.0", "json": "^11.0.0", "prettier": "^3.2.5", diff --git a/tests/vanilla/memoryleaks.test.ts b/tests/vanilla/memoryleaks.test.ts new file mode 100644 index 0000000000..8ea9db249c --- /dev/null +++ b/tests/vanilla/memoryleaks.test.ts @@ -0,0 +1,14 @@ +import LeakDetector from 'jest-leak-detector' +import { expect, it } from 'vitest' +import { atom, createStore } from 'jotai/vanilla' + +it('should not have memory leaks with an atom', async () => { + const store = createStore() + let detector: LeakDetector + ;(() => { + const objAtom = atom({}) + detector = new LeakDetector(store.get(objAtom)) + })() + const isLeaking = await detector.isLeaking() + expect(isLeaking).toBe(false) +}) diff --git a/yarn.lock b/yarn.lock index b2d84a1b28..8e0a4fbb55 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3466,6 +3466,19 @@ iterator.prototype@^1.1.2: reflect.getprototypeof "^1.0.4" set-function-name "^2.0.1" +jest-get-type@^29.6.3: + version "29.6.3" + resolved "https://registry.yarnpkg.com/jest-get-type/-/jest-get-type-29.6.3.tgz#36f499fdcea197c1045a127319c0481723908fd1" + integrity sha512-zrteXnqYxfQh7l5FHyL38jL39di8H8rHoecLH3JNxH3BwOrBsNeabdap5e0I23lD4HHI8W5VFBZqG4Eaq5LNcw== + +jest-leak-detector@^29.7.0: + version "29.7.0" + resolved "https://registry.yarnpkg.com/jest-leak-detector/-/jest-leak-detector-29.7.0.tgz#5b7ec0dadfdfec0ca383dc9aa016d36b5ea4c728" + integrity sha512-kYA8IJcSYtST2BY9I+SMC32nDpBT3J2NvWJx8+JCuCdl/CR1I4EKUJROiP8XtCcxqgTTBGJNdbB1A8XRKbTetw== + dependencies: + jest-get-type "^29.6.3" + pretty-format "^29.7.0" + "js-tokens@^3.0.0 || ^4.0.0", js-tokens@^4.0.0: version "4.0.0" resolved "https://registry.yarnpkg.com/js-tokens/-/js-tokens-4.0.0.tgz#19203fb59991df98e3a287050d4647cdeaf32499"