From 8bfef177a2d2851138018f7e1337c43e76a6304b Mon Sep 17 00:00:00 2001 From: MadCcc <1075746765@qq.com> Date: Wed, 9 Aug 2023 21:34:18 +0800 Subject: [PATCH 1/6] fix: should cleanup styles in useEffect cleanup function --- src/hooks/useCacheToken.tsx | 25 +++-- src/hooks/useEffectCleanupRegister.ts | 48 ++++++++++ tests/index.spec.tsx | 132 +++++++++++++++++++++----- 3 files changed, 174 insertions(+), 31 deletions(-) create mode 100644 src/hooks/useEffectCleanupRegister.ts diff --git a/src/hooks/useCacheToken.tsx b/src/hooks/useCacheToken.tsx index 135a8fb..f0d731b 100644 --- a/src/hooks/useCacheToken.tsx +++ b/src/hooks/useCacheToken.tsx @@ -4,6 +4,7 @@ import { useContext } from 'react'; import StyleContext, { ATTR_TOKEN, CSS_IN_JS_INSTANCE } from '../StyleContext'; import type Theme from '../theme/Theme'; import { flattenToken, token2key } from '../util'; +import useEffectCleanupRegister from './useEffectCleanupRegister'; import useGlobalCache from './useGlobalCache'; const EMPTY_OVERRIDE = {}; @@ -40,7 +41,11 @@ export interface Option { * @param override Extra tokens to override. * @param theme Theme instance. Could get derivative token by `theme.getDerivativeToken` */ - getComputedToken?: (origin: DesignToken, override: object, theme: Theme) => DerivativeToken; + getComputedToken?: ( + origin: DesignToken, + override: object, + theme: Theme, + ) => DerivativeToken; } const tokenKeys = new Map(); @@ -129,9 +134,11 @@ export default function useCacheToken< salt = '', override = EMPTY_OVERRIDE, formatToken, - getComputedToken: compute + getComputedToken: compute, } = option; + const register = useEffectCleanupRegister(); + // Basic - We do basic cache here const mergedToken = React.useMemo( () => Object.assign({}, ...tokens), @@ -152,12 +159,9 @@ export default function useCacheToken< 'token', [salt, theme.id, tokenStr, overrideTokenStr], () => { - const mergedDerivativeToken = compute ? compute(mergedToken, override, theme) : getComputedToken( - mergedToken, - override, - theme, - formatToken, - ); + const mergedDerivativeToken = compute + ? compute(mergedToken, override, theme) + : getComputedToken(mergedToken, override, theme, formatToken); // Optimize for `useStyleRegister` performance const tokenKey = token2key(mergedDerivativeToken, salt); @@ -171,7 +175,10 @@ export default function useCacheToken< }, (cache) => { // Remove token will remove all related style - cleanTokenStyle(cache[0]._tokenKey, instanceId); + // Always remove styles in useEffect callback + register(() => { + cleanTokenStyle(cache[0]._tokenKey, instanceId); + }); }, ); diff --git a/src/hooks/useEffectCleanupRegister.ts b/src/hooks/useEffectCleanupRegister.ts new file mode 100644 index 0000000..a15ade4 --- /dev/null +++ b/src/hooks/useEffectCleanupRegister.ts @@ -0,0 +1,48 @@ +import { warning } from 'rc-util/lib/warning'; +import * as React from 'react'; + +const fullClone = { + ...React, +}; +const { useInsertionEffect } = fullClone; + +// DO NOT register functions in useEffect cleanup function, or functions that registered will never be called. +const useCleanupRegister = () => { + const effectCleanups: (() => void)[] = []; + let cleanupFlag = false; + function register(fn: () => void) { + if (cleanupFlag) { + if (process.env.NODE_ENV !== 'production') { + warning( + false, + '[Ant Design CSS-in-JS] You are registering a cleanup function after unmount, which will not have any effect.', + ); + } + return; + } + effectCleanups.push(fn); + } + + React.useEffect(() => () => { + cleanupFlag = true; + if (effectCleanups.length) { + for (const fn of effectCleanups) { + fn(); + } + } + }); + + return register; +}; + +const useRun = () => { + return function (fn: () => void) { + fn(); + }; +}; + +// Only enable register in React 18 +const useEffectCleanupRegister = + typeof useInsertionEffect !== 'undefined' ? useCleanupRegister : useRun; + +export default useEffectCleanupRegister; diff --git a/tests/index.spec.tsx b/tests/index.spec.tsx index feb3c11..32398c4 100644 --- a/tests/index.spec.tsx +++ b/tests/index.spec.tsx @@ -1,17 +1,18 @@ import { render } from '@testing-library/react'; import classNames from 'classnames'; import * as React from 'react'; -import type { CSSInterpolation } from '../src'; +import { ReactNode } from 'react'; +import { expect } from 'vitest'; +import type { CSSInterpolation, DerivativeFunc } from '../src'; import { createCache, + createTheme, StyleProvider, Theme, useCacheToken, useStyleRegister, - createTheme } from '../src'; import { ATTR_MARK, ATTR_TOKEN, CSS_IN_JS_INSTANCE } from '../src/StyleContext'; -import type { DerivativeFunc } from '../src'; interface DesignToken { primaryColor: string; @@ -562,22 +563,32 @@ describe('csssinjs', () => { }, }); - const Demo = ({myToken, theme: customTheme}: { myToken?: string, theme?: DerivativeFunc }) => { - const [token, hashId] = useCacheToken(theme, [{primaryColor: 'blue'}], { - salt: 'test', - override: { - myToken, - theme: customTheme && createTheme(customTheme) + const Demo = ({ + myToken, + theme: customTheme, + }: { + myToken?: string; + theme?: DerivativeFunc; + }) => { + const [token, hashId] = useCacheToken( + theme, + [{ primaryColor: 'blue' }], + { + salt: 'test', + override: { + myToken, + theme: customTheme && createTheme(customTheme), + }, + getComputedToken: (origin, override: any, myTheme) => { + const mergedToken = myTheme.getDerivativeToken(origin); + return { + ...mergedToken, + myToken: override.myToken, + ...(override.theme?.getDerivativeToken(mergedToken) ?? {}), + }; + }, }, - getComputedToken: (origin, override: any, myTheme) => { - const mergedToken = myTheme.getDerivativeToken(origin); - return { - ...mergedToken, - myToken: override.myToken, - ...(override.theme?.getDerivativeToken(mergedToken) ?? {}), - } - } - }); + ); useStyleRegister( { theme, token, hashId, path: ['cssinjs-getComputedToken'] }, @@ -587,7 +598,7 @@ describe('csssinjs', () => { return
; }; - const { rerender } =render(); + const { rerender } = render(); const styles = Array.from(document.head.querySelectorAll('style')); expect(styles).toHaveLength(1); @@ -601,11 +612,88 @@ describe('csssinjs', () => { expect(styles2[0].innerHTML).toContain('color:apple'); expect(styles2[0].innerHTML).toContain('background:blue'); - rerender( ({...origin, primaryColor: 'green'})} />); + rerender( + ({ ...origin, primaryColor: 'green' })} + />, + ); const styles3 = Array.from(document.head.querySelectorAll('style')); expect(styles3).toHaveLength(1); expect(styles3[0].innerHTML).toContain('color:banana'); - expect(styles3[0].innerHTML).toContain('background:green'); - }) + expect(styles3[0].innerHTML).toContain('b1ackground:green'); + }); + + it.only('should not cleanup token before finishing rendering', () => { + const spy = vi.spyOn(console, 'error').mockImplementation(() => {}); + const genDemoStyle = (token: any): CSSInterpolation => ({ + '.box': { + color: token.primaryColor, + }, + }); + + const Style = ({ token, hashId }: { token: any; hashId: string }) => { + useStyleRegister( + { + theme, + token, + hashId, + path: ['cssinjs-cleanup-token-after-render', hashId], + }, + () => [genDemoStyle(token)], + ); + + return null; + }; + + const Demo = ({ + myToken, + children, + }: { + myToken?: string; + children?: ReactNode; + }) => { + const [token, hashId] = useCacheToken( + theme, + [{ primaryColor: myToken }], + { + salt: 'test', + }, + ); + + return ( + <> +