From fa5d4ee43b2b7145ab4013b86662e69ea64ab180 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 5 Mar 2019 21:07:37 +0000 Subject: [PATCH 01/22] [ESLint] Treat functions that don't capture anything as static (#14996) * Treat functions that don't capture anything as static * Fix comment --- .../ESLintRuleExhaustiveDeps-test.js | 507 ++++++++++++++++++ .../src/ExhaustiveDeps.js | 245 ++++++--- 2 files changed, 674 insertions(+), 78 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index 8dd36917b0679..348221cd2bdae 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -652,6 +652,206 @@ const tests = { } `, }, + { + code: ` + function MyComponent(props) { + function handleNext1() { + console.log('hello'); + } + const handleNext2 = () => { + console.log('hello'); + }; + let handleNext3 = function() { + console.log('hello'); + }; + useEffect(() => { + return Store.subscribe(handleNext1); + }, [handleNext1]); + useLayoutEffect(() => { + return Store.subscribe(handleNext2); + }, [handleNext2]); + useMemo(() => { + return Store.subscribe(handleNext3); + }, [handleNext3]); + } + `, + }, + { + // Declaring handleNext is optional because + // it doesn't use anything in the function scope. + code: ` + function MyComponent(props) { + function handleNext1() { + console.log('hello'); + } + const handleNext2 = () => { + console.log('hello'); + }; + let handleNext3 = function() { + console.log('hello'); + }; + useEffect(() => { + return Store.subscribe(handleNext1); + }, []); + useLayoutEffect(() => { + return Store.subscribe(handleNext2); + }, []); + useMemo(() => { + return Store.subscribe(handleNext3); + }, []); + } + `, + }, + { + // Declaring handleNext is optional because + // it doesn't use anything in the function scope. + code: ` + function MyComponent(props) { + function handleNext() { + console.log('hello'); + } + useEffect(() => { + return Store.subscribe(handleNext); + }, []); + useLayoutEffect(() => { + return Store.subscribe(handleNext); + }, []); + useMemo(() => { + return Store.subscribe(handleNext); + }, []); + } + `, + }, + { + // Declaring handleNext is optional because + // everything they use is fully static. + code: ` + function MyComponent(props) { + let [, setState] = useState(); + let [, dispatch] = React.useReducer(); + + function handleNext1(value) { + let value2 = value * 100; + setState(value2); + console.log('hello'); + } + const handleNext2 = (value) => { + setState(foo(value)); + console.log('hello'); + }; + let handleNext3 = function(value) { + console.log(value); + dispatch({ type: 'x', value }); + }; + useEffect(() => { + return Store.subscribe(handleNext1); + }, []); + useLayoutEffect(() => { + return Store.subscribe(handleNext2); + }, []); + useMemo(() => { + return Store.subscribe(handleNext3); + }, []); + } + `, + }, + { + code: ` + function useInterval(callback, delay) { + const savedCallback = useRef(); + useEffect(() => { + savedCallback.current = callback; + }); + useEffect(() => { + function tick() { + savedCallback.current(); + } + if (delay !== null) { + let id = setInterval(tick, delay); + return () => clearInterval(id); + } + }, [delay]); + } + `, + }, + { + code: ` + function Counter() { + const [count, setCount] = useState(0); + + useEffect(() => { + let id = setInterval(() => { + setCount(c => c + 1); + }, 1000); + return () => clearInterval(id); + }, []); + + return

{count}

; + } + `, + }, + { + code: ` + function Counter() { + const [count, setCount] = useState(0); + + function tick() { + setCount(c => c + 1); + } + + useEffect(() => { + let id = setInterval(() => { + tick(); + }, 1000); + return () => clearInterval(id); + }, []); + + return

{count}

; + } + `, + }, + { + code: ` + function Counter() { + const [count, dispatch] = useReducer((state, action) => { + if (action === 'inc') { + return state + 1; + } + }, 0); + + useEffect(() => { + let id = setInterval(() => { + dispatch('inc'); + }, 1000); + return () => clearInterval(id); + }, []); + + return

{count}

; + } + `, + }, + { + code: ` + function Counter() { + const [count, dispatch] = useReducer((state, action) => { + if (action === 'inc') { + return state + 1; + } + }, 0); + + const tick = () => { + dispatch('inc'); + }; + + useEffect(() => { + let id = setInterval(tick, 1000); + return () => clearInterval(id); + }, []); + + return

{count}

; + } + `, + }, ], invalid: [ { @@ -2858,6 +3058,313 @@ const tests = { "because their mutation doesn't re-render the component.", ], }, + { + // Every almost-static function is tainted by a dynamic value. + code: ` + function MyComponent(props) { + let [, setState] = useState(); + let [, dispatch] = React.useReducer(); + let taint = props.foo; + + function handleNext1(value) { + let value2 = value * taint; + setState(value2); + console.log('hello'); + } + const handleNext2 = (value) => { + setState(taint(value)); + console.log('hello'); + }; + let handleNext3 = function(value) { + setTimeout(() => console.log(taint)); + dispatch({ type: 'x', value }); + }; + useEffect(() => { + return Store.subscribe(handleNext1); + }, []); + useLayoutEffect(() => { + return Store.subscribe(handleNext2); + }, []); + useMemo(() => { + return Store.subscribe(handleNext3); + }, []); + } + `, + output: ` + function MyComponent(props) { + let [, setState] = useState(); + let [, dispatch] = React.useReducer(); + let taint = props.foo; + + function handleNext1(value) { + let value2 = value * taint; + setState(value2); + console.log('hello'); + } + const handleNext2 = (value) => { + setState(taint(value)); + console.log('hello'); + }; + let handleNext3 = function(value) { + setTimeout(() => console.log(taint)); + dispatch({ type: 'x', value }); + }; + useEffect(() => { + return Store.subscribe(handleNext1); + }, [handleNext1]); + useLayoutEffect(() => { + return Store.subscribe(handleNext2); + }, [handleNext2]); + useMemo(() => { + return Store.subscribe(handleNext3); + }, [handleNext3]); + } + `, + errors: [ + "React Hook useEffect has a missing dependency: 'handleNext1'. " + + 'Either include it or remove the dependency array.', + "React Hook useLayoutEffect has a missing dependency: 'handleNext2'. " + + 'Either include it or remove the dependency array.', + "React Hook useMemo has a missing dependency: 'handleNext3'. " + + 'Either include it or remove the dependency array.', + ], + }, + { + // Regression test + code: ` + function MyComponent(props) { + let [, setState] = useState(); + let [, dispatch] = React.useReducer(); + let taint = props.foo; + + // Shouldn't affect anything + function handleChange() {} + + function handleNext1(value) { + let value2 = value * taint; + setState(value2); + console.log('hello'); + } + const handleNext2 = (value) => { + setState(taint(value)); + console.log('hello'); + }; + let handleNext3 = function(value) { + console.log(taint); + dispatch({ type: 'x', value }); + }; + useEffect(() => { + return Store.subscribe(handleNext1); + }, []); + useLayoutEffect(() => { + return Store.subscribe(handleNext2); + }, []); + useMemo(() => { + return Store.subscribe(handleNext3); + }, []); + } + `, + output: ` + function MyComponent(props) { + let [, setState] = useState(); + let [, dispatch] = React.useReducer(); + let taint = props.foo; + + // Shouldn't affect anything + function handleChange() {} + + function handleNext1(value) { + let value2 = value * taint; + setState(value2); + console.log('hello'); + } + const handleNext2 = (value) => { + setState(taint(value)); + console.log('hello'); + }; + let handleNext3 = function(value) { + console.log(taint); + dispatch({ type: 'x', value }); + }; + useEffect(() => { + return Store.subscribe(handleNext1); + }, [handleNext1]); + useLayoutEffect(() => { + return Store.subscribe(handleNext2); + }, [handleNext2]); + useMemo(() => { + return Store.subscribe(handleNext3); + }, [handleNext3]); + } + `, + errors: [ + "React Hook useEffect has a missing dependency: 'handleNext1'. " + + 'Either include it or remove the dependency array.', + "React Hook useLayoutEffect has a missing dependency: 'handleNext2'. " + + 'Either include it or remove the dependency array.', + "React Hook useMemo has a missing dependency: 'handleNext3'. " + + 'Either include it or remove the dependency array.', + ], + }, + { + // Regression test + code: ` + function MyComponent(props) { + let [, setState] = useState(); + let [, dispatch] = React.useReducer(); + let taint = props.foo; + + // Shouldn't affect anything + const handleChange = () => {}; + + function handleNext1(value) { + let value2 = value * taint; + setState(value2); + console.log('hello'); + } + const handleNext2 = (value) => { + setState(taint(value)); + console.log('hello'); + }; + let handleNext3 = function(value) { + console.log(taint); + dispatch({ type: 'x', value }); + }; + useEffect(() => { + return Store.subscribe(handleNext1); + }, []); + useLayoutEffect(() => { + return Store.subscribe(handleNext2); + }, []); + useMemo(() => { + return Store.subscribe(handleNext3); + }, []); + } + `, + output: ` + function MyComponent(props) { + let [, setState] = useState(); + let [, dispatch] = React.useReducer(); + let taint = props.foo; + + // Shouldn't affect anything + const handleChange = () => {}; + + function handleNext1(value) { + let value2 = value * taint; + setState(value2); + console.log('hello'); + } + const handleNext2 = (value) => { + setState(taint(value)); + console.log('hello'); + }; + let handleNext3 = function(value) { + console.log(taint); + dispatch({ type: 'x', value }); + }; + useEffect(() => { + return Store.subscribe(handleNext1); + }, [handleNext1]); + useLayoutEffect(() => { + return Store.subscribe(handleNext2); + }, [handleNext2]); + useMemo(() => { + return Store.subscribe(handleNext3); + }, [handleNext3]); + } + `, + errors: [ + "React Hook useEffect has a missing dependency: 'handleNext1'. " + + 'Either include it or remove the dependency array.', + "React Hook useLayoutEffect has a missing dependency: 'handleNext2'. " + + 'Either include it or remove the dependency array.', + "React Hook useMemo has a missing dependency: 'handleNext3'. " + + 'Either include it or remove the dependency array.', + ], + }, + { + code: ` + function Counter() { + let [count, setCount] = useState(0); + + useEffect(() => { + let id = setInterval(() => { + setCount(count + 1); + }, 1000); + return () => clearInterval(id); + }, []); + + return

{count}

; + } + `, + output: ` + function Counter() { + let [count, setCount] = useState(0); + + useEffect(() => { + let id = setInterval(() => { + setCount(count + 1); + }, 1000); + return () => clearInterval(id); + }, [count]); + + return

{count}

; + } + `, + // TODO: ideally this should suggest useState updater form + // since this code doesn't actually work. + errors: [ + "React Hook useEffect has a missing dependency: 'count'. " + + 'Either include it or remove the dependency array.', + ], + }, + { + code: ` + function Counter() { + const [count, setCount] = useState(0); + + function tick() { + setCount(count + 1); + } + + useEffect(() => { + let id = setInterval(() => { + tick(); + }, 1000); + return () => clearInterval(id); + }, []); + + return

{count}

; + } + `, + output: ` + function Counter() { + const [count, setCount] = useState(0); + + function tick() { + setCount(count + 1); + } + + useEffect(() => { + let id = setInterval(() => { + tick(); + }, 1000); + return () => clearInterval(id); + }, [tick]); + + return

{count}

; + } + `, + // TODO: ideally this should suggest useState updater form + // since this code doesn't actually work. The autofix could + // at least avoid suggesting 'tick' since it's obviously + // always different, and thus useless. + errors: [ + "React Hook useEffect has a missing dependency: 'tick'. " + + 'Either include it or remove the dependency array.', + ], + }, ], }; diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index 9fd60d960e45c..58a5d2a25dff7 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -34,6 +34,22 @@ export default { : undefined; const options = {additionalHooks}; + // Should be shared between visitors. + let staticKnownValueCache = new WeakMap(); + let functionWithoutCapturedValueCache = new WeakMap(); + function memoizeWithWeakMap(fn, map) { + return function(arg) { + if (map.has(arg)) { + // to verify cache hits: + // console.log(arg.name) + return map.get(arg); + } + const result = fn(arg); + map.set(arg, result); + return result; + }; + } + return { FunctionExpression: visitFunctionExpression, ArrowFunctionExpression: visitFunctionExpression, @@ -105,6 +121,153 @@ export default { componentScope = currentScope; } + // Next we'll define a few helpers that helps us + // tell if some values don't have to be declared as deps. + + // Some are known to be static based on Hook calls. + // const [state, setState] = useState() / React.useState() + // ^^^ true for this reference + // const [state, dispatch] = useReducer() / React.useReducer() + // ^^^ true for this reference + // const ref = useRef() + // ^^^ true for this reference + // False for everything else. + function isStaticKnownHookValue(resolved) { + if (!Array.isArray(resolved.defs)) { + return false; + } + const def = resolved.defs[0]; + if (def == null) { + return false; + } + // Look for `let stuff = ...` + if (def.node.type !== 'VariableDeclarator') { + return false; + } + const init = def.node.init; + if (init == null) { + return false; + } + // Detect primitive constants + // const foo = 42 + const declaration = def.node.parent; + if ( + declaration.kind === 'const' && + init.type === 'Literal' && + (typeof init.value === 'string' || + typeof init.value === 'number' || + init.value === null) + ) { + // Definitely static + return true; + } + // Detect known Hook calls + // const [_, setState] = useState() + if (init.type !== 'CallExpression') { + return false; + } + let callee = init.callee; + // Step into `= React.something` initializer. + if ( + callee.type === 'MemberExpression' && + callee.object.name === 'React' && + callee.property != null && + !callee.computed + ) { + callee = callee.property; + } + if (callee.type !== 'Identifier') { + return false; + } + const id = def.node.id; + if (callee.name === 'useRef' && id.type === 'Identifier') { + // useRef() return value is static. + return true; + } else if (callee.name === 'useState' || callee.name === 'useReducer') { + // Only consider second value in initializing tuple static. + if ( + id.type === 'ArrayPattern' && + id.elements.length === 2 && + Array.isArray(resolved.identifiers) && + // Is second tuple value the same reference we're checking? + id.elements[1] === resolved.identifiers[0] + ) { + return true; + } + } + // By default assume it's dynamic. + return false; + } + + // Some are just functions that don't reference anything dynamic. + function isFunctionWithoutCapturedValues(resolved) { + if (!Array.isArray(resolved.defs)) { + return false; + } + const def = resolved.defs[0]; + if (def == null) { + return false; + } + if (def.node == null || def.node.id == null) { + return false; + } + // Search the direct component subscopes for + // top-level function definitions matching this reference. + const fnNode = def.node; + let childScopes = componentScope.childScopes; + let fnScope = null; + let i; + for (i = 0; i < childScopes.length; i++) { + let childScope = childScopes[i]; + let childScopeBlock = childScope.block; + if ( + // function handleChange() {} + (fnNode.type === 'FunctionDeclaration' && + childScopeBlock === fnNode) || + // const handleChange = () => {} + // const handleChange = function() {} + (fnNode.type === 'VariableDeclarator' && + childScopeBlock.parent === fnNode) + ) { + // Found it! + fnScope = childScope; + break; + } + } + if (fnScope == null) { + return false; + } + // Does this function capture any values + // that are in pure scopes (aka render)? + for (i = 0; i < fnScope.through.length; i++) { + const ref = fnScope.through[i]; + if (ref.resolved == null) { + continue; + } + if ( + pureScopes.has(ref.resolved.scope) && + // Static values are fine though, + // although we won't check functions deeper. + !memoizedIsStaticKnownHookValue(ref.resolved) + ) { + return false; + } + } + // If we got here, this function doesn't capture anything + // from render--or everything it captures is known static. + return true; + } + + // Remember such values. Avoid re-running extra checks on them. + const memoizedIsStaticKnownHookValue = memoizeWithWeakMap( + isStaticKnownHookValue, + staticKnownValueCache, + ); + const memoizedIsFunctionWithoutCapturedValues = memoizeWithWeakMap( + isFunctionWithoutCapturedValues, + functionWithoutCapturedValueCache, + ); + // These are usually mistaken. Collect them. const currentRefsInEffectCleanup = new Map(); @@ -172,7 +335,10 @@ export default { // Add the dependency to a map so we can make sure it is referenced // again in our dependencies array. Remember whether it's static. if (!dependencies.has(dependency)) { - const isStatic = isDefinitelyStaticDependency(reference); + const resolved = reference.resolved; + const isStatic = + memoizedIsStaticKnownHookValue(resolved) || + memoizedIsFunctionWithoutCapturedValues(resolved); dependencies.set(dependency, { isStatic, reference, @@ -778,83 +944,6 @@ function getReactiveHookCallbackIndex(calleeNode, options) { } } -// const [state, setState] = useState() / React.useState() -// ^^^ true for this reference -// const [state, dispatch] = useReducer() / React.useReducer() -// ^^^ true for this reference -// const ref = useRef() -// ^^^ true for this reference -// False for everything else. -function isDefinitelyStaticDependency(reference) { - // This function is written defensively because I'm not sure about corner cases. - // TODO: we can strengthen this if we're sure about the types. - const resolved = reference.resolved; - if (resolved == null || !Array.isArray(resolved.defs)) { - return false; - } - const def = resolved.defs[0]; - if (def == null) { - return false; - } - // Look for `let stuff = ...` - if (def.node.type !== 'VariableDeclarator') { - return false; - } - const init = def.node.init; - if (init == null) { - return false; - } - // Detect primitive constants - // const foo = 42 - const declaration = def.node.parent; - if ( - declaration.kind === 'const' && - init.type === 'Literal' && - (typeof init.value === 'string' || - typeof init.value === 'number' || - init.value === null) - ) { - // Definitely static - return true; - } - // Detect known Hook calls - // const [_, setState] = useState() - if (init.type !== 'CallExpression') { - return false; - } - let callee = init.callee; - // Step into `= React.something` initializer. - if ( - callee.type === 'MemberExpression' && - callee.object.name === 'React' && - callee.property != null && - !callee.computed - ) { - callee = callee.property; - } - if (callee.type !== 'Identifier') { - return false; - } - const id = def.node.id; - if (callee.name === 'useRef' && id.type === 'Identifier') { - // useRef() return value is static. - return true; - } else if (callee.name === 'useState' || callee.name === 'useReducer') { - // Only consider second value in initializing tuple static. - if ( - id.type === 'ArrayPattern' && - id.elements.length === 2 && - Array.isArray(reference.resolved.identifiers) && - // Is second tuple value the same reference we're checking? - id.elements[1] === reference.resolved.identifiers[0] - ) { - return true; - } - } - // By default assume it's dynamic. - return false; -} - /** * ESLint won't assign node.parent to references from context.getScope() * From db8d466554a1a85bf9427fb04c1901503d65f93f Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 5 Mar 2019 22:40:04 +0000 Subject: [PATCH 02/22] Fix heading in changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index aefe0f5410051..360eaac201463 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,7 +16,7 @@ ### React DOM Server * Unwind the context stack when a stream is destroyed without completing, to prevent incorrect values during a subsequent render. ([@overlookmotel](https://github.com/overlookmotel) in [#14706](https://github.com/facebook/react/pull/14706/)) -## ESLint Plugin for React Hooks +### ESLint Plugin for React Hooks * Add a new recommended `exhaustive-deps` rule. ([@gaearon](https://github.com/gaearon) in [#14636](https://github.com/facebook/react/pull/14636)) From a9aa24ed8dc843b76ceb4880b83e776c696cafaf Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 5 Mar 2019 15:16:01 -0800 Subject: [PATCH 03/22] 16.8.4 and changelog --- CHANGELOG.md | 6 ++++++ packages/create-subscription/package.json | 2 +- packages/jest-react/package.json | 2 +- packages/react-art/package.json | 4 ++-- packages/react-dom/package.json | 4 ++-- packages/react-is/package.json | 2 +- packages/react-reconciler/package.json | 4 ++-- packages/react-test-renderer/package.json | 6 +++--- packages/react/package.json | 4 ++-- packages/scheduler/package.json | 2 +- packages/shared/ReactVersion.js | 2 +- 11 files changed, 22 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 360eaac201463..052c07aa33fdb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,12 @@ +## 16.8.4 (March 5, 2019) + +### React DOM and other renderers + +- Fix a bug where DevTools caused a runtime error when inspecting a component that used a `useContext` hook. ([@bvaughn](https://github.com/bvaughn) in [#14940](https://github.com/facebook/react/pull/14940)) + ## 16.8.3 (February 21, 2019) ### React DOM diff --git a/packages/create-subscription/package.json b/packages/create-subscription/package.json index ec15a9cb2a09c..5230f5beac976 100644 --- a/packages/create-subscription/package.json +++ b/packages/create-subscription/package.json @@ -1,7 +1,7 @@ { "name": "create-subscription", "description": "utility for subscribing to external data sources inside React components", - "version": "16.8.3", + "version": "16.8.4", "repository": { "type": "git", "url": "https://github.com/facebook/react.git", diff --git a/packages/jest-react/package.json b/packages/jest-react/package.json index 5d669186085e4..65cdd7f0ab0bd 100644 --- a/packages/jest-react/package.json +++ b/packages/jest-react/package.json @@ -1,6 +1,6 @@ { "name": "jest-react", - "version": "0.6.3", + "version": "0.6.4", "description": "Jest matchers and utilities for testing React components.", "main": "index.js", "repository": { diff --git a/packages/react-art/package.json b/packages/react-art/package.json index 414b423cc0247..5489654088853 100644 --- a/packages/react-art/package.json +++ b/packages/react-art/package.json @@ -1,7 +1,7 @@ { "name": "react-art", "description": "React ART is a JavaScript library for drawing vector graphics using React. It provides declarative and reactive bindings to the ART library. Using the same declarative API you can render the output to either Canvas, SVG or VML (IE8).", - "version": "16.8.3", + "version": "16.8.4", "main": "index.js", "repository": { "type": "git", @@ -27,7 +27,7 @@ "loose-envify": "^1.1.0", "object-assign": "^4.1.1", "prop-types": "^15.6.2", - "scheduler": "^0.13.3" + "scheduler": "^0.13.4" }, "peerDependencies": { "react": "^16.0.0" diff --git a/packages/react-dom/package.json b/packages/react-dom/package.json index 375099986b59e..e430bef026870 100644 --- a/packages/react-dom/package.json +++ b/packages/react-dom/package.json @@ -1,6 +1,6 @@ { "name": "react-dom", - "version": "16.8.3", + "version": "16.8.4", "description": "React package for working with the DOM.", "main": "index.js", "repository": { @@ -20,7 +20,7 @@ "loose-envify": "^1.1.0", "object-assign": "^4.1.1", "prop-types": "^15.6.2", - "scheduler": "^0.13.3" + "scheduler": "^0.13.4" }, "peerDependencies": { "react": "^16.0.0" diff --git a/packages/react-is/package.json b/packages/react-is/package.json index e13eea0056f21..1aec135c4b84e 100644 --- a/packages/react-is/package.json +++ b/packages/react-is/package.json @@ -1,6 +1,6 @@ { "name": "react-is", - "version": "16.8.3", + "version": "16.8.4", "description": "Brand checking of React Elements.", "main": "index.js", "repository": { diff --git a/packages/react-reconciler/package.json b/packages/react-reconciler/package.json index 6740798e3d998..8be62150a9810 100644 --- a/packages/react-reconciler/package.json +++ b/packages/react-reconciler/package.json @@ -1,7 +1,7 @@ { "name": "react-reconciler", "description": "React package for creating custom renderers.", - "version": "0.20.1", + "version": "0.20.2", "keywords": [ "react" ], @@ -33,7 +33,7 @@ "loose-envify": "^1.1.0", "object-assign": "^4.1.1", "prop-types": "^15.6.2", - "scheduler": "^0.13.3" + "scheduler": "^0.13.4" }, "browserify": { "transform": [ diff --git a/packages/react-test-renderer/package.json b/packages/react-test-renderer/package.json index c69f38c8f78ee..e71d6e77d3727 100644 --- a/packages/react-test-renderer/package.json +++ b/packages/react-test-renderer/package.json @@ -1,6 +1,6 @@ { "name": "react-test-renderer", - "version": "16.8.3", + "version": "16.8.4", "description": "React package for snapshot testing.", "main": "index.js", "repository": { @@ -21,8 +21,8 @@ "dependencies": { "object-assign": "^4.1.1", "prop-types": "^15.6.2", - "react-is": "^16.8.3", - "scheduler": "^0.13.3" + "react-is": "^16.8.4", + "scheduler": "^0.13.4" }, "peerDependencies": { "react": "^16.0.0" diff --git a/packages/react/package.json b/packages/react/package.json index 22230761e86b0..dadd7d2da5e40 100644 --- a/packages/react/package.json +++ b/packages/react/package.json @@ -4,7 +4,7 @@ "keywords": [ "react" ], - "version": "16.8.3", + "version": "16.8.4", "homepage": "https://reactjs.org/", "bugs": "https://github.com/facebook/react/issues", "license": "MIT", @@ -29,7 +29,7 @@ "loose-envify": "^1.1.0", "object-assign": "^4.1.1", "prop-types": "^15.6.2", - "scheduler": "^0.13.3" + "scheduler": "^0.13.4" }, "browserify": { "transform": [ diff --git a/packages/scheduler/package.json b/packages/scheduler/package.json index dde926df057e4..a0a3e286af7ef 100644 --- a/packages/scheduler/package.json +++ b/packages/scheduler/package.json @@ -1,6 +1,6 @@ { "name": "scheduler", - "version": "0.13.3", + "version": "0.13.4", "description": "Cooperative scheduler for the browser environment.", "main": "index.js", "repository": { diff --git a/packages/shared/ReactVersion.js b/packages/shared/ReactVersion.js index 27f8681f10ff3..c5f23d432ba93 100644 --- a/packages/shared/ReactVersion.js +++ b/packages/shared/ReactVersion.js @@ -8,4 +8,4 @@ 'use strict'; // TODO: this is special because it gets imported during build. -module.exports = '16.8.3'; +module.exports = '16.8.4'; From 5d49dafac80733fbd8699138c766da3339468254 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 6 Mar 2019 18:17:54 +0000 Subject: [PATCH 04/22] Enforce deps array in useMemo and useCallback (#15025) --- .../ESLintRuleExhaustiveDeps-test.js | 30 +++++++++++++++++++ .../src/ExhaustiveDeps.js | 12 ++++++++ 2 files changed, 42 insertions(+) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index 348221cd2bdae..79713c57cdec8 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -178,6 +178,16 @@ const tests = { } `, }, + { + // Valid because they have meaning without deps. + code: ` + function MyComponent(props) { + useEffect(() => {}); + useLayoutEffect(() => {}); + useImperativeHandle(props.innerRef, () => {}); + } + `, + }, { code: ` function MyComponent(props) { @@ -924,6 +934,26 @@ const tests = { 'Either include it or remove the dependency array.', ], }, + { + // Invalid because they don't have a meaning without deps. + code: ` + function MyComponent(props) { + const value = useMemo(() => { return 2*2; }); + const fn = useCallback(() => { alert('foo'); }); + } + `, + // We don't know what you meant. + output: ` + function MyComponent(props) { + const value = useMemo(() => { return 2*2; }); + const fn = useCallback(() => { alert('foo'); }); + } + `, + errors: [ + "React Hook useMemo doesn't serve any purpose without a dependency array as a second argument.", + "React Hook useCallback doesn't serve any purpose without a dependency array as a second argument.", + ], + }, { // Regression test code: ` diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index 58a5d2a25dff7..51efab5f5dee7 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -87,6 +87,18 @@ export default { const depsIndex = callbackIndex + 1; const declaredDependenciesNode = node.parent.arguments[depsIndex]; if (!declaredDependenciesNode) { + // These are only used for optimization. + if ( + reactiveHookName === 'useMemo' || + reactiveHookName === 'useCallback' + ) { + context.report({ + node: node, + message: + `React Hook ${reactiveHookName} doesn't serve any purpose ` + + `without a dependency array as a second argument.`, + }); + } return; } From 1e3b6192b54df20ac117a2af56afbe00ac9487b7 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 6 Mar 2019 14:41:45 -0800 Subject: [PATCH 05/22] Import Scheduler directly, not via host config (#14984) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Import Scheduler directly, not via host config We currently schedule asynchronous tasks via the host config. (The host config is a static/build-time dependency injection system that varies across different renderers — DOM, native, test, and so on.) Instead of calling platform APIs like `requestIdleCallback` directly, each renderer implements a method called `scheduleDeferredCallback`. We've since discovered that when scheduling tasks, it's crucial that React work is placed in the same queue as other, non-React work on the main thread. Otherwise, you easily end up in a starvation scenario where rendering is constantly interrupted by less important tasks. You need a centralized coordinator that is used both by React and by other frameworks and application code. This coordinator must also have a consistent API across all the different host environments, for convention's sake and so product code is portable — e.g. so the same component can work in both React Native and React Native Web. This turned into the Scheduler package. We will have different builds of Scheduler for each of our target platforms. With this approach, we treat Scheduler like a built-in platform primitive that exists wherever React is supported. Now that we have this consistent interface, the indirection of the host config no longer makes sense for the purpose of scheduling tasks. In fact, we explicitly do not want renderers to scheduled task via any system except the Scheduler package. So, this PR removes `scheduleDeferredCallback` and its associated methods from the host config in favor of directly importing Scheduler. * Missed an extraneous export --- packages/react-art/src/ReactARTHostConfig.js | 2 - .../src/client/ReactDOMHostConfig.js | 2 - .../src/ReactFabricHostConfig.js | 12 ---- .../src/ReactNativeFrameScheduling.js | 56 ------------------- .../src/ReactNativeHostConfig.js | 12 ---- .../src/createReactNoop.js | 8 --- .../src/ReactFiberScheduler.js | 27 ++++----- .../src/ReactProfilerTimer.js | 6 +- .../src/forks/ReactFiberHostConfig.custom.js | 5 -- .../src/ReactTestHostConfig.js | 9 --- .../__tests__/ReactProfiler-test.internal.js | 8 ++- 11 files changed, 24 insertions(+), 123 deletions(-) delete mode 100644 packages/react-native-renderer/src/ReactNativeFrameScheduling.js diff --git a/packages/react-art/src/ReactARTHostConfig.js b/packages/react-art/src/ReactARTHostConfig.js index 64114982cc379..7f0196d9f6645 100644 --- a/packages/react-art/src/ReactARTHostConfig.js +++ b/packages/react-art/src/ReactARTHostConfig.js @@ -343,8 +343,6 @@ export function getChildHostContext() { export const scheduleTimeout = setTimeout; export const cancelTimeout = clearTimeout; export const noTimeout = -1; -export const schedulePassiveEffects = scheduleDeferredCallback; -export const cancelPassiveEffects = cancelDeferredCallback; export function shouldSetTextContent(type, props) { return ( diff --git a/packages/react-dom/src/client/ReactDOMHostConfig.js b/packages/react-dom/src/client/ReactDOMHostConfig.js index 3467711921a1c..1576b6ae9ff92 100644 --- a/packages/react-dom/src/client/ReactDOMHostConfig.js +++ b/packages/react-dom/src/client/ReactDOMHostConfig.js @@ -310,8 +310,6 @@ export const scheduleTimeout = export const cancelTimeout = typeof clearTimeout === 'function' ? clearTimeout : (undefined: any); export const noTimeout = -1; -export const schedulePassiveEffects = scheduleDeferredCallback; -export const cancelPassiveEffects = cancelDeferredCallback; // ------------------- // Mutation diff --git a/packages/react-native-renderer/src/ReactFabricHostConfig.js b/packages/react-native-renderer/src/ReactFabricHostConfig.js index a1e0b882c6ef6..bb4646b190bce 100644 --- a/packages/react-native-renderer/src/ReactFabricHostConfig.js +++ b/packages/react-native-renderer/src/ReactFabricHostConfig.js @@ -20,12 +20,6 @@ import { warnForStyleProps, } from './NativeMethodsMixinUtils'; import {create, diff} from './ReactNativeAttributePayload'; -import { - now as ReactNativeFrameSchedulingNow, - cancelDeferredCallback as ReactNativeFrameSchedulingCancelDeferredCallback, - scheduleDeferredCallback as ReactNativeFrameSchedulingScheduleDeferredCallback, - shouldYield as ReactNativeFrameSchedulingShouldYield, -} from './ReactNativeFrameScheduling'; import {get as getViewConfigForType} from 'ReactNativeViewConfigRegistry'; import deepFreezeAndThrowOnMutationInDev from 'deepFreezeAndThrowOnMutationInDev'; @@ -333,16 +327,10 @@ export function shouldSetTextContent(type: string, props: Props): boolean { // The Fabric renderer is secondary to the existing React Native renderer. export const isPrimaryRenderer = false; -export const now = ReactNativeFrameSchedulingNow; -export const scheduleDeferredCallback = ReactNativeFrameSchedulingScheduleDeferredCallback; -export const cancelDeferredCallback = ReactNativeFrameSchedulingCancelDeferredCallback; -export const shouldYield = ReactNativeFrameSchedulingShouldYield; export const scheduleTimeout = setTimeout; export const cancelTimeout = clearTimeout; export const noTimeout = -1; -export const schedulePassiveEffects = scheduleDeferredCallback; -export const cancelPassiveEffects = cancelDeferredCallback; // ------------------- // Persistence diff --git a/packages/react-native-renderer/src/ReactNativeFrameScheduling.js b/packages/react-native-renderer/src/ReactNativeFrameScheduling.js deleted file mode 100644 index 3c11c0d20ac79..0000000000000 --- a/packages/react-native-renderer/src/ReactNativeFrameScheduling.js +++ /dev/null @@ -1,56 +0,0 @@ -/** - * Copyright (c) Facebook, Inc. and its affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - * - * @flow - */ - -const hasNativePerformanceNow = - typeof performance === 'object' && typeof performance.now === 'function'; - -const now = hasNativePerformanceNow - ? () => performance.now() - : () => Date.now(); - -let scheduledCallback: (() => mixed) | null = null; -let frameDeadline: number = 0; - -function setTimeoutCallback() { - // TODO (bvaughn) Hard-coded 5ms unblocks initial async testing. - // React API probably changing to boolean rather than time remaining. - // Longer-term plan is to rewrite this using shared memory, - // And just return the value of the bit as the boolean. - frameDeadline = now() + 5; - - const callback = scheduledCallback; - scheduledCallback = null; - if (callback !== null) { - callback(); - } -} - -// RN has a poor polyfill for requestIdleCallback so we aren't using it. -// This implementation is only intended for short-term use anyway. -// We also don't implement cancel functionality b'c Fiber doesn't currently need it. -function scheduleDeferredCallback( - callback: () => mixed, - options?: {timeout: number}, -): number { - // We assume only one callback is scheduled at a time b'c that's how Fiber works. - scheduledCallback = callback; - const timeoutId = setTimeout(setTimeoutCallback, 1); - return (timeoutId: any); // Timeouts are always numbers on RN -} - -function cancelDeferredCallback(callbackID: number) { - scheduledCallback = null; - clearTimeout((callbackID: any)); // Timeouts are always numbers on RN -} - -function shouldYield() { - return frameDeadline <= now(); -} - -export {now, scheduleDeferredCallback, cancelDeferredCallback, shouldYield}; diff --git a/packages/react-native-renderer/src/ReactNativeHostConfig.js b/packages/react-native-renderer/src/ReactNativeHostConfig.js index b16a3174eba91..dca90571b0c37 100644 --- a/packages/react-native-renderer/src/ReactNativeHostConfig.js +++ b/packages/react-native-renderer/src/ReactNativeHostConfig.js @@ -23,12 +23,6 @@ import { updateFiberProps, } from './ReactNativeComponentTree'; import ReactNativeFiberHostComponent from './ReactNativeFiberHostComponent'; -import { - now as ReactNativeFrameSchedulingNow, - cancelDeferredCallback as ReactNativeFrameSchedulingCancelDeferredCallback, - scheduleDeferredCallback as ReactNativeFrameSchedulingScheduleDeferredCallback, - shouldYield as ReactNativeFrameSchedulingShouldYield, -} from './ReactNativeFrameScheduling'; export type Type = string; export type Props = Object; @@ -234,17 +228,11 @@ export function resetAfterCommit(containerInfo: Container): void { // Noop } -export const now = ReactNativeFrameSchedulingNow; export const isPrimaryRenderer = true; -export const scheduleDeferredCallback = ReactNativeFrameSchedulingScheduleDeferredCallback; -export const cancelDeferredCallback = ReactNativeFrameSchedulingCancelDeferredCallback; -export const shouldYield = ReactNativeFrameSchedulingShouldYield; export const scheduleTimeout = setTimeout; export const cancelTimeout = clearTimeout; export const noTimeout = -1; -export const schedulePassiveEffects = scheduleDeferredCallback; -export const cancelPassiveEffects = cancelDeferredCallback; export function shouldDeprioritizeSubtree(type: string, props: Props): boolean { return false; diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index ab1a93ad4ce66..b24bb68a2a8d1 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -304,18 +304,10 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { return inst; }, - scheduleDeferredCallback: Scheduler.unstable_scheduleCallback, - cancelDeferredCallback: Scheduler.unstable_cancelCallback, - - shouldYield: Scheduler.unstable_shouldYield, - scheduleTimeout: setTimeout, cancelTimeout: clearTimeout, noTimeout: -1, - schedulePassiveEffects: Scheduler.unstable_scheduleCallback, - cancelPassiveEffects: Scheduler.unstable_cancelCallback, - prepareForCommit(): void {}, resetAfterCommit(): void {}, diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index be5062139ab13..390f46af4ea4c 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -14,9 +14,7 @@ import type {Interaction} from 'scheduler/src/Tracing'; // Intentionally not named imports because Rollup would use dynamic dispatch for // CommonJS interop named imports. -// TODO: We're not using this import anymore, but I've left this here so we -// don't accidentally use named imports when we add it back. -// import * as Scheduler from 'scheduler'; +import * as Scheduler from 'scheduler'; import { __interactionsRef, __subscriberRef, @@ -78,17 +76,11 @@ import { setCurrentFiber, } from './ReactCurrentFiber'; import { - now, - scheduleDeferredCallback, - cancelDeferredCallback, - shouldYield, prepareForCommit, resetAfterCommit, scheduleTimeout, cancelTimeout, noTimeout, - schedulePassiveEffects, - cancelPassiveEffects, } from './ReactFiberHostConfig'; import { markPendingPriorityLevel, @@ -172,6 +164,15 @@ import { } from './ReactFiberCommitWork'; import {ContextOnlyDispatcher} from './ReactFiberHooks'; +// Intentionally not named imports because Rollup would use dynamic dispatch for +// CommonJS interop named imports. +const { + unstable_scheduleCallback: scheduleCallback, + unstable_cancelCallback: cancelCallback, + unstable_shouldYield: shouldYield, + unstable_now: now, +} = Scheduler; + export type Thenable = { then(resolve: () => mixed, reject?: () => mixed): mixed, }; @@ -598,7 +599,7 @@ function markLegacyErrorBoundaryAsFailed(instance: mixed) { function flushPassiveEffects() { if (passiveEffectCallbackHandle !== null) { - cancelPassiveEffects(passiveEffectCallbackHandle); + cancelCallback(passiveEffectCallbackHandle); } if (passiveEffectCallback !== null) { // We call the scheduled callback instead of commitPassiveEffects directly @@ -807,7 +808,7 @@ function commitRoot(root: FiberRoot, finishedWork: Fiber): void { // here because that code is still in flux. callback = Scheduler_tracing_wrap(callback); } - passiveEffectCallbackHandle = schedulePassiveEffects(callback); + passiveEffectCallbackHandle = scheduleCallback(callback); passiveEffectCallback = callback; } @@ -1978,7 +1979,7 @@ function scheduleCallbackWithExpirationTime( if (callbackID !== null) { // Existing callback has insufficient timeout. Cancel and schedule a // new one. - cancelDeferredCallback(callbackID); + cancelCallback(callbackID); } } // The request callback timer is already running. Don't start a new one. @@ -1990,7 +1991,7 @@ function scheduleCallbackWithExpirationTime( const currentMs = now() - originalStartTimeMs; const expirationTimeMs = expirationTimeToMs(expirationTime); const timeout = expirationTimeMs - currentMs; - callbackID = scheduleDeferredCallback(performAsyncWork, {timeout}); + callbackID = scheduleCallback(performAsyncWork, {timeout}); } // For every call to renderRoot, one of onFatal, onComplete, onSuspend, and diff --git a/packages/react-reconciler/src/ReactProfilerTimer.js b/packages/react-reconciler/src/ReactProfilerTimer.js index 5b78822ca647d..6aa1642dfe6c0 100644 --- a/packages/react-reconciler/src/ReactProfilerTimer.js +++ b/packages/react-reconciler/src/ReactProfilerTimer.js @@ -11,7 +11,11 @@ import type {Fiber} from './ReactFiber'; import {enableProfilerTimer} from 'shared/ReactFeatureFlags'; -import {now} from './ReactFiberHostConfig'; +// Intentionally not named imports because Rollup would use dynamic dispatch for +// CommonJS interop named imports. +import * as Scheduler from 'scheduler'; + +const {unstable_now: now} = Scheduler; export type ProfilerTimer = { getCommitTime(): number, diff --git a/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js b/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js index 7dde223171587..994ca852ad327 100644 --- a/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js +++ b/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js @@ -51,14 +51,9 @@ export const shouldSetTextContent = $$$hostConfig.shouldSetTextContent; export const shouldDeprioritizeSubtree = $$$hostConfig.shouldDeprioritizeSubtree; export const createTextInstance = $$$hostConfig.createTextInstance; -export const scheduleDeferredCallback = $$$hostConfig.scheduleDeferredCallback; -export const cancelDeferredCallback = $$$hostConfig.cancelDeferredCallback; -export const shouldYield = $$$hostConfig.shouldYield; export const scheduleTimeout = $$$hostConfig.setTimeout; export const cancelTimeout = $$$hostConfig.clearTimeout; export const noTimeout = $$$hostConfig.noTimeout; -export const schedulePassiveEffects = $$$hostConfig.schedulePassiveEffects; -export const cancelPassiveEffects = $$$hostConfig.cancelPassiveEffects; export const now = $$$hostConfig.now; export const isPrimaryRenderer = $$$hostConfig.isPrimaryRenderer; export const supportsMutation = $$$hostConfig.supportsMutation; diff --git a/packages/react-test-renderer/src/ReactTestHostConfig.js b/packages/react-test-renderer/src/ReactTestHostConfig.js index f8d98bc0e18b8..0e4f1d8e0d437 100644 --- a/packages/react-test-renderer/src/ReactTestHostConfig.js +++ b/packages/react-test-renderer/src/ReactTestHostConfig.js @@ -7,7 +7,6 @@ * @flow */ -import * as Scheduler from 'scheduler/unstable_mock'; import warning from 'shared/warning'; export type Type = string; @@ -195,18 +194,10 @@ export function createTextInstance( } export const isPrimaryRenderer = false; -// This approach enables `now` to be mocked by tests, -// Even after the reconciler has initialized and read host config values. -export const now = Scheduler.unstable_now; -export const scheduleDeferredCallback = Scheduler.unstable_scheduleCallback; -export const cancelDeferredCallback = Scheduler.unstable_cancelCallback; -export const shouldYield = Scheduler.unstable_shouldYield; export const scheduleTimeout = setTimeout; export const cancelTimeout = clearTimeout; export const noTimeout = -1; -export const schedulePassiveEffects = Scheduler.unstable_scheduleCallback; -export const cancelPassiveEffects = Scheduler.unstable_cancelCallback; // ------------------- // Mutation diff --git a/packages/react/src/__tests__/ReactProfiler-test.internal.js b/packages/react/src/__tests__/ReactProfiler-test.internal.js index 7d8bcf65108cc..42fbb387dd096 100644 --- a/packages/react/src/__tests__/ReactProfiler-test.internal.js +++ b/packages/react/src/__tests__/ReactProfiler-test.internal.js @@ -261,7 +261,7 @@ describe('Profiler', () => { it('does not record times for components outside of Profiler tree', () => { // Mock the Scheduler module so we can track how many times the current // time is read - jest.mock('scheduler/unstable_mock', obj => { + jest.mock('scheduler', obj => { const ActualScheduler = require.requireActual( 'scheduler/unstable_mock', ); @@ -300,8 +300,10 @@ describe('Profiler', () => { 'read current time', ]); - // Remove mock - jest.unmock('scheduler/unstable_mock'); + // Restore original mock + jest.mock('scheduler', () => + require.requireActual('scheduler/unstable_mock'), + ); }); it('logs render times for both mount and update', () => { From 9b7e1d1389e080d19e71680bbbe979ec58fa7389 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 6 Mar 2019 23:50:02 +0000 Subject: [PATCH 06/22] [ESLint] Suggest moving inside a Hook or useCallback when bare function is a dependency (#15026) * Warn about bare function deps and suggest moving or useCallback * Clearer wording --- .../ESLintRuleExhaustiveDeps-test.js | 479 ++++++++++++++++-- .../src/ExhaustiveDeps.js | 133 ++++- 2 files changed, 579 insertions(+), 33 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index 79713c57cdec8..0c3bd909ca253 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -313,7 +313,7 @@ const tests = { }, { code: ` - function MyComponent({ maybeRef2 }) { + function MyComponent({ maybeRef2, foo }) { const definitelyRef1 = useRef(); const definitelyRef2 = useRef(); const maybeRef1 = useSomeOtherRefyThing(); @@ -323,8 +323,8 @@ const tests = { const [state4, dispatch2] = React.useReducer(); const [state5, maybeSetState] = useFunnyState(); const [state6, maybeDispatch] = useFunnyReducer(); - function mySetState() {} - function myDispatch() {} + const mySetState = useCallback(() => {}, []); + let myDispatch = useCallback(() => {}, []); useEffect(() => { // Known to be static @@ -380,8 +380,8 @@ const tests = { const [state5, maybeSetState] = useFunnyState(); const [state6, maybeDispatch] = useFunnyReducer(); - function mySetState() {} - function myDispatch() {} + const mySetState = useCallback(() => {}, []); + let myDispatch = useCallback(() => {}, []); useEffect(() => { // Known to be static @@ -662,30 +662,6 @@ const tests = { } `, }, - { - code: ` - function MyComponent(props) { - function handleNext1() { - console.log('hello'); - } - const handleNext2 = () => { - console.log('hello'); - }; - let handleNext3 = function() { - console.log('hello'); - }; - useEffect(() => { - return Store.subscribe(handleNext1); - }, [handleNext1]); - useLayoutEffect(() => { - return Store.subscribe(handleNext2); - }, [handleNext2]); - useMemo(() => { - return Store.subscribe(handleNext3); - }, [handleNext3]); - } - `, - }, { // Declaring handleNext is optional because // it doesn't use anything in the function scope. @@ -950,8 +926,12 @@ const tests = { } `, errors: [ - "React Hook useMemo doesn't serve any purpose without a dependency array as a second argument.", - "React Hook useCallback doesn't serve any purpose without a dependency array as a second argument.", + "React Hook useMemo doesn't serve any purpose without a dependency array. " + + 'To enable this optimization, pass an array of values used by the inner ' + + 'function as the second argument to useMemo.', + "React Hook useCallback doesn't serve any purpose without a dependency array. " + + 'To enable this optimization, pass an array of values used by the inner ' + + 'function as the second argument to useCallback.', ], }, { @@ -3313,6 +3293,443 @@ const tests = { 'Either include it or remove the dependency array.', ], }, + { + // Even if the function only references static values, + // once you specify it in deps, it will invalidate them. + code: ` + function MyComponent(props) { + let [, setState] = useState(); + + function handleNext(value) { + setState(value); + } + + useEffect(() => { + return Store.subscribe(handleNext); + }, [handleNext]); + } + `, + // Not gonna autofix a function definition + // because it's not always safe due to hoisting. + output: ` + function MyComponent(props) { + let [, setState] = useState(); + + function handleNext(value) { + setState(value); + } + + useEffect(() => { + return Store.subscribe(handleNext); + }, [handleNext]); + } + `, + errors: [ + `The 'handleNext' function makes the dependencies of ` + + `useEffect Hook (at line 11) change on every render. ` + + `To fix this, move the 'handleNext' function ` + + `inside the useEffect callback. Alternatively, ` + + `wrap the 'handleNext' definition into its own useCallback() Hook.`, + ], + }, + { + // Even if the function only references static values, + // once you specify it in deps, it will invalidate them. + code: ` + function MyComponent(props) { + let [, setState] = useState(); + + const handleNext = (value) => { + setState(value); + }; + + useEffect(() => { + return Store.subscribe(handleNext); + }, [handleNext]); + } + `, + // We don't autofix moving (too invasive). But that's the suggested fix + // when only effect uses this function. Otherwise, we'd useCallback. + output: ` + function MyComponent(props) { + let [, setState] = useState(); + + const handleNext = (value) => { + setState(value); + }; + + useEffect(() => { + return Store.subscribe(handleNext); + }, [handleNext]); + } + `, + errors: [ + `The 'handleNext' function makes the dependencies of ` + + `useEffect Hook (at line 11) change on every render. ` + + `To fix this, move the 'handleNext' function ` + + `inside the useEffect callback. Alternatively, ` + + `wrap the 'handleNext' definition into its own useCallback() Hook.`, + ], + }, + { + // Even if the function only references static values, + // once you specify it in deps, it will invalidate them. + // However, we can't suggest moving handleNext into the + // effect because it is *also* used outside of it. + // So our suggestion is useCallback(). + code: ` + function MyComponent(props) { + let [, setState] = useState(); + + const handleNext = (value) => { + setState(value); + }; + + useEffect(() => { + return Store.subscribe(handleNext); + }, [handleNext]); + + return
; + } + `, + // We autofix this one with useCallback since it's + // the easy fix and you can't just move it into effect. + output: ` + function MyComponent(props) { + let [, setState] = useState(); + + const handleNext = useCallback((value) => { + setState(value); + }); + + useEffect(() => { + return Store.subscribe(handleNext); + }, [handleNext]); + + return
; + } + `, + errors: [ + `The 'handleNext' function makes the dependencies of ` + + `useEffect Hook (at line 11) change on every render. ` + + `To fix this, wrap the 'handleNext' definition into its own useCallback() Hook.`, + ], + }, + { + code: ` + function MyComponent(props) { + function handleNext1() { + console.log('hello'); + } + const handleNext2 = () => { + console.log('hello'); + }; + let handleNext3 = function() { + console.log('hello'); + }; + useEffect(() => { + return Store.subscribe(handleNext1); + }, [handleNext1]); + useLayoutEffect(() => { + return Store.subscribe(handleNext2); + }, [handleNext2]); + useMemo(() => { + return Store.subscribe(handleNext3); + }, [handleNext3]); + } + `, + // Autofix doesn't wrap into useCallback here + // because they are only referenced by effect itself. + output: ` + function MyComponent(props) { + function handleNext1() { + console.log('hello'); + } + const handleNext2 = () => { + console.log('hello'); + }; + let handleNext3 = function() { + console.log('hello'); + }; + useEffect(() => { + return Store.subscribe(handleNext1); + }, [handleNext1]); + useLayoutEffect(() => { + return Store.subscribe(handleNext2); + }, [handleNext2]); + useMemo(() => { + return Store.subscribe(handleNext3); + }, [handleNext3]); + } + `, + errors: [ + "The 'handleNext1' function makes the dependencies of useEffect Hook " + + "(at line 14) change on every render. To fix this, move the 'handleNext1' " + + 'function inside the useEffect callback. Alternatively, wrap the ' + + "'handleNext1' definition into its own useCallback() Hook.", + "The 'handleNext2' function makes the dependencies of useLayoutEffect Hook " + + "(at line 17) change on every render. To fix this, move the 'handleNext2' " + + 'function inside the useLayoutEffect callback. Alternatively, wrap the ' + + "'handleNext2' definition into its own useCallback() Hook.", + "The 'handleNext3' function makes the dependencies of useMemo Hook " + + "(at line 20) change on every render. To fix this, move the 'handleNext3' " + + 'function inside the useMemo callback. Alternatively, wrap the ' + + "'handleNext3' definition into its own useCallback() Hook.", + ], + }, + { + code: ` + function MyComponent(props) { + function handleNext1() { + console.log('hello'); + } + const handleNext2 = () => { + console.log('hello'); + }; + let handleNext3 = function() { + console.log('hello'); + }; + useEffect(() => { + handleNext1(); + return Store.subscribe(() => handleNext1()); + }, [handleNext1]); + useLayoutEffect(() => { + handleNext2(); + return Store.subscribe(() => handleNext2()); + }, [handleNext2]); + useMemo(() => { + handleNext3(); + return Store.subscribe(() => handleNext3()); + }, [handleNext3]); + } + `, + // Autofix doesn't wrap into useCallback here + // because they are only referenced by effect itself. + output: ` + function MyComponent(props) { + function handleNext1() { + console.log('hello'); + } + const handleNext2 = () => { + console.log('hello'); + }; + let handleNext3 = function() { + console.log('hello'); + }; + useEffect(() => { + handleNext1(); + return Store.subscribe(() => handleNext1()); + }, [handleNext1]); + useLayoutEffect(() => { + handleNext2(); + return Store.subscribe(() => handleNext2()); + }, [handleNext2]); + useMemo(() => { + handleNext3(); + return Store.subscribe(() => handleNext3()); + }, [handleNext3]); + } + `, + errors: [ + "The 'handleNext1' function makes the dependencies of useEffect Hook " + + "(at line 15) change on every render. To fix this, move the 'handleNext1' " + + 'function inside the useEffect callback. Alternatively, wrap the ' + + "'handleNext1' definition into its own useCallback() Hook.", + "The 'handleNext2' function makes the dependencies of useLayoutEffect Hook " + + "(at line 19) change on every render. To fix this, move the 'handleNext2' " + + 'function inside the useLayoutEffect callback. Alternatively, wrap the ' + + "'handleNext2' definition into its own useCallback() Hook.", + "The 'handleNext3' function makes the dependencies of useMemo Hook " + + "(at line 23) change on every render. To fix this, move the 'handleNext3' " + + 'function inside the useMemo callback. Alternatively, wrap the ' + + "'handleNext3' definition into its own useCallback() Hook.", + ], + }, + { + code: ` + function MyComponent(props) { + function handleNext1() { + console.log('hello'); + } + const handleNext2 = () => { + console.log('hello'); + }; + let handleNext3 = function() { + console.log('hello'); + }; + useEffect(() => { + handleNext1(); + return Store.subscribe(() => handleNext1()); + }, [handleNext1]); + useLayoutEffect(() => { + handleNext2(); + return Store.subscribe(() => handleNext2()); + }, [handleNext2]); + useMemo(() => { + handleNext3(); + return Store.subscribe(() => handleNext3()); + }, [handleNext3]); + return ( +
{ + handleNext1(); + setTimeout(handleNext2); + setTimeout(() => { + handleNext3(); + }); + }} + /> + ); + } + `, + // Autofix wraps into useCallback where possible (variables only) + // because they are only referenced outside the effect. + output: ` + function MyComponent(props) { + function handleNext1() { + console.log('hello'); + } + const handleNext2 = useCallback(() => { + console.log('hello'); + }); + let handleNext3 = useCallback(function() { + console.log('hello'); + }); + useEffect(() => { + handleNext1(); + return Store.subscribe(() => handleNext1()); + }, [handleNext1]); + useLayoutEffect(() => { + handleNext2(); + return Store.subscribe(() => handleNext2()); + }, [handleNext2]); + useMemo(() => { + handleNext3(); + return Store.subscribe(() => handleNext3()); + }, [handleNext3]); + return ( +
{ + handleNext1(); + setTimeout(handleNext2); + setTimeout(() => { + handleNext3(); + }); + }} + /> + ); + } + `, + errors: [ + "The 'handleNext1' function makes the dependencies of useEffect Hook " + + '(at line 15) change on every render. To fix this, wrap the ' + + "'handleNext1' definition into its own useCallback() Hook.", + "The 'handleNext2' function makes the dependencies of useLayoutEffect Hook " + + '(at line 19) change on every render. To fix this, wrap the ' + + "'handleNext2' definition into its own useCallback() Hook.", + "The 'handleNext3' function makes the dependencies of useMemo Hook " + + '(at line 23) change on every render. To fix this, wrap the ' + + "'handleNext3' definition into its own useCallback() Hook.", + ], + }, + { + code: ` + function MyComponent(props) { + const handleNext1 = () => { + console.log('hello'); + }; + function handleNext2() { + console.log('hello'); + } + useEffect(() => { + return Store.subscribe(handleNext1); + return Store.subscribe(handleNext2); + }, [handleNext1, handleNext2]); + useEffect(() => { + return Store.subscribe(handleNext1); + return Store.subscribe(handleNext2); + }, [handleNext1, handleNext2]); + } + `, + // Normally we'd suggest moving handleNext inside an + // effect. But it's used by more than one. So we + // suggest useCallback() and use it for the autofix + // where possible (variable but not declaration). + output: ` + function MyComponent(props) { + const handleNext1 = useCallback(() => { + console.log('hello'); + }); + function handleNext2() { + console.log('hello'); + } + useEffect(() => { + return Store.subscribe(handleNext1); + return Store.subscribe(handleNext2); + }, [handleNext1, handleNext2]); + useEffect(() => { + return Store.subscribe(handleNext1); + return Store.subscribe(handleNext2); + }, [handleNext1, handleNext2]); + } + `, + // TODO: we could coalesce messages for the same function if it affects multiple Hooks. + errors: [ + "The 'handleNext1' function makes the dependencies of useEffect Hook " + + '(at line 12) change on every render. To fix this, wrap the ' + + "'handleNext1' definition into its own useCallback() Hook.", + "The 'handleNext1' function makes the dependencies of useEffect Hook " + + '(at line 16) change on every render. To fix this, wrap the ' + + "'handleNext1' definition into its own useCallback() Hook.", + "The 'handleNext2' function makes the dependencies of useEffect Hook " + + '(at line 12) change on every render. To fix this, wrap the ' + + "'handleNext2' definition into its own useCallback() Hook.", + "The 'handleNext2' function makes the dependencies of useEffect Hook " + + '(at line 16) change on every render. To fix this, wrap the ' + + "'handleNext2' definition into its own useCallback() Hook.", + ], + }, + { + code: ` + function MyComponent(props) { + let [, setState] = useState(); + let taint = props.foo; + + function handleNext(value) { + let value2 = value * taint; + setState(value2); + console.log('hello'); + } + + useEffect(() => { + return Store.subscribe(handleNext); + }, [handleNext]); + } + `, + output: ` + function MyComponent(props) { + let [, setState] = useState(); + let taint = props.foo; + + function handleNext(value) { + let value2 = value * taint; + setState(value2); + console.log('hello'); + } + + useEffect(() => { + return Store.subscribe(handleNext); + }, [handleNext]); + } + `, + errors: [ + `The 'handleNext' function makes the dependencies of ` + + `useEffect Hook (at line 14) change on every render. ` + + `To fix this, move the 'handleNext' function inside ` + + `the useEffect callback. Alternatively, wrap the ` + + `'handleNext' definition into its own useCallback() Hook.`, + ], + }, { code: ` function Counter() { diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index 51efab5f5dee7..252f29a703d0d 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -93,10 +93,12 @@ export default { reactiveHookName === 'useCallback' ) { context.report({ - node: node, + node: node.parent.callee, message: `React Hook ${reactiveHookName} doesn't serve any purpose ` + - `without a dependency array as a second argument.`, + `without a dependency array. To enable ` + + `this optimization, pass an array of values used by the ` + + `inner function as the second argument to ${reactiveHookName}.`, }); } return; @@ -550,7 +552,57 @@ export default { duplicateDependencies.size + missingDependencies.size + unnecessaryDependencies.size; + if (problemCount === 0) { + // If nothing else to report, check if some callbacks + // are bare and would invalidate on every render. + const bareFunctions = scanForDeclaredBareFunctions({ + declaredDependencies, + declaredDependenciesNode, + componentScope, + scope, + }); + bareFunctions.forEach(({fn, suggestUseCallback}) => { + let message = + `The '${fn.name.name}' function makes the dependencies of ` + + `${reactiveHookName} Hook (at line ${ + declaredDependenciesNode.loc.start.line + }) ` + + `change on every render.`; + if (suggestUseCallback) { + message += + ` To fix this, ` + + `wrap the '${ + fn.name.name + }' definition into its own useCallback() Hook.`; + } else { + message += + ` To fix this, move the '${fn.name.name}' function ` + + `inside the ${reactiveHookName} callback. Alternatively, ` + + `wrap the '${ + fn.name.name + }' definition into its own useCallback() Hook.`; + } + context.report({ + node: fn.node, + message, + fix(fixer) { + // Only handle the simple case: arrow functions. + // Wrapping function declarations can mess up hoisting. + if (suggestUseCallback && fn.type === 'Variable') { + return [ + // TODO: also add an import? + fixer.insertTextBefore(fn.node.init, 'useCallback('), + // TODO: ideally we'd gather deps here but it would + // require restructuring the rule code. For now, + // this is fine. Note we're intentionally not adding + // [] because that changes semantics. + fixer.insertTextAfter(fn.node.init, ')'), + ]; + } + }, + }); + }); return; } @@ -858,6 +910,83 @@ function collectRecommendations({ }; } +// Finds functions declared as dependencies +// that would invalidate on every render. +function scanForDeclaredBareFunctions({ + declaredDependencies, + declaredDependenciesNode, + componentScope, + scope, +}) { + const bareFunctions = declaredDependencies + .map(({key}) => { + const fnRef = componentScope.set.get(key); + if (fnRef == null) { + return null; + } + let fnNode = fnRef.defs[0]; + if (fnNode == null) { + return null; + } + // const handleChange = function () {} + // const handleChange = () => {} + if ( + fnNode.type === 'Variable' && + fnNode.node.type === 'VariableDeclarator' && + fnNode.node.init != null && + (fnNode.node.init.type === 'ArrowFunctionExpression' || + fnNode.node.init.type === 'FunctionExpression') + ) { + return fnRef; + } + // function handleChange() {} + if ( + fnNode.type === 'FunctionName' && + fnNode.node.type === 'FunctionDeclaration' + ) { + return fnRef; + } + return null; + }) + .filter(Boolean); + + function isUsedOutsideOfHook(fnRef) { + let foundWriteExpr = false; + for (let i = 0; i < fnRef.references.length; i++) { + const reference = fnRef.references[i]; + if (reference.writeExpr) { + if (foundWriteExpr) { + // Two writes to the same function. + return true; + } else { + // Ignore first write as it's not usage. + foundWriteExpr = true; + continue; + } + } + let currentScope = reference.from; + while (currentScope !== scope && currentScope != null) { + currentScope = currentScope.upper; + } + if (currentScope !== scope) { + // This reference is outside the Hook callback. + // It can only be legit if it's the deps array. + if (isAncestorNodeOf(declaredDependenciesNode, reference.identifier)) { + continue; + } else { + return true; + } + } + } + return false; + } + + return bareFunctions.map(fnRef => ({ + fn: fnRef.defs[0], + suggestUseCallback: isUsedOutsideOfHook(fnRef), + })); +} + /** * Assuming () means the passed/returned node: * (props) => (props) From 6d2666bab16ee8c822e848f84a6feb0d42a6d78c Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 7 Mar 2019 00:39:39 +0000 Subject: [PATCH 07/22] Fix ESLint rule crash (#15044) --- .../ESLintRuleExhaustiveDeps-test.js | 37 +++++++++++++++++++ .../src/ExhaustiveDeps.js | 12 +++++- 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index 0c3bd909ca253..a1c7932368507 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -838,6 +838,17 @@ const tests = { } `, }, + { + // Regression test for a crash + code: ` + function Podcasts() { + useEffect(() => { + setPodcasts([]); + }, []); + let [podcasts, setPodcasts] = useState(null); + } + `, + }, ], invalid: [ { @@ -3812,6 +3823,32 @@ const tests = { 'Either include it or remove the dependency array.', ], }, + { + // Regression test for a crash + code: ` + function Podcasts() { + useEffect(() => { + alert(podcasts); + }, []); + let [podcasts, setPodcasts] = useState(null); + } + `, + // Note: this autofix is shady because + // the variable is used before declaration. + // TODO: Maybe we can catch those fixes and not autofix. + output: ` + function Podcasts() { + useEffect(() => { + alert(podcasts); + }, [podcasts]); + let [podcasts, setPodcasts] = useState(null); + } + `, + errors: [ + `React Hook useEffect has a missing dependency: 'podcasts'. ` + + `Either include it or remove the dependency array.`, + ], + }, ], }; diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index 252f29a703d0d..2f8b18c0069a7 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -164,7 +164,17 @@ export default { } // Detect primitive constants // const foo = 42 - const declaration = def.node.parent; + let declaration = def.node.parent; + if (declaration == null) { + // This might happen if variable is declared after the callback. + // In that case ESLint won't set up .parent refs. + // So we'll set them up manually. + fastFindReferenceWithParent(componentScope.block, def.node.id); + declaration = def.node.parent; + if (declaration == null) { + return false; + } + } if ( declaration.kind === 'const' && init.type === 'Literal' && From 197703ecc776dd5c1a4d956896603bcc67fb9920 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 7 Mar 2019 12:39:15 +0000 Subject: [PATCH 08/22] [ESLint] Add more hints to lint messages (#15046) * A clearer message for props destructuring where applicable * Add line number to the "move function" message * Add a hint for how to fix callbacks from props * Simplify code and harden tests * Collect all dependency references for better warnings * Suggest updater or reducer where appropriate --- .../ESLintRuleExhaustiveDeps-test.js | 568 +++++++++++++++++- .../src/ExhaustiveDeps.js | 220 ++++++- 2 files changed, 744 insertions(+), 44 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index a1c7932368507..c30146cac3635 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -849,6 +849,106 @@ const tests = { } `, }, + { + code: ` + function withFetch(fetchPodcasts) { + return function Podcasts({ id }) { + let [podcasts, setPodcasts] = useState(null); + useEffect(() => { + fetchPodcasts(id).then(setPodcasts); + }, [id]); + } + } + `, + }, + { + code: ` + function Podcasts({ id }) { + let [podcasts, setPodcasts] = useState(null); + useEffect(() => { + function doFetch({ fetchPodcasts }) { + fetchPodcasts(id).then(setPodcasts); + } + doFetch({ fetchPodcasts: API.fetchPodcasts }); + }, [id]); + } + `, + }, + { + code: ` + function Counter() { + let [count, setCount] = useState(0); + + function increment(x) { + return x + 1; + } + + useEffect(() => { + let id = setInterval(() => { + setCount(increment); + }, 1000); + return () => clearInterval(id); + }, []); + + return

{count}

; + } + `, + }, + { + code: ` + function Counter() { + let [count, setCount] = useState(0); + + function increment(x) { + return x + 1; + } + + useEffect(() => { + let id = setInterval(() => { + setCount(count => increment(count)); + }, 1000); + return () => clearInterval(id); + }, []); + + return

{count}

; + } + `, + }, + { + code: ` + import increment from './increment'; + function Counter() { + let [count, setCount] = useState(0); + + useEffect(() => { + let id = setInterval(() => { + setCount(count => count + increment); + }, 1000); + return () => clearInterval(id); + }, []); + + return

{count}

; + } + `, + }, + { + code: ` + function withStuff(increment) { + return function Counter() { + let [count, setCount] = useState(0); + + useEffect(() => { + let id = setInterval(() => { + setCount(count => count + increment); + }, 1000); + return () => clearInterval(id); + }, []); + + return

{count}

; + } + } + `, + }, ], invalid: [ { @@ -2203,7 +2303,10 @@ const tests = { `, errors: [ "React Hook useEffect has a missing dependency: 'state'. " + - 'Either include it or remove the dependency array.', + 'Either include it or remove the dependency array. ' + + `If 'state' is only necessary for calculating the next state, ` + + `consider refactoring to the setState(state => ...) form which ` + + `doesn't need to depend on the state from outside.`, ], }, { @@ -2232,7 +2335,10 @@ const tests = { `, errors: [ "React Hook useEffect has a missing dependency: 'state'. " + - 'Either include it or remove the dependency array.', + 'Either include it or remove the dependency array. ' + + `If 'state' is only necessary for calculating the next state, ` + + `consider refactoring to the setState(state => ...) form which ` + + `doesn't need to depend on the state from outside.`, ], }, { @@ -2447,7 +2553,9 @@ const tests = { errors: [ "React Hook useEffect has a missing dependency: 'props'. " + 'Either include it or remove the dependency array. ' + - 'Alternatively, destructure the necessary props outside the callback.', + `However, the preferred fix is to destructure the 'props' ` + + `object outside of the useEffect call and refer to specific ` + + `props directly by their names.`, ], }, { @@ -2478,7 +2586,9 @@ const tests = { errors: [ "React Hook useEffect has a missing dependency: 'props'. " + 'Either include it or remove the dependency array. ' + - 'Alternatively, destructure the necessary props outside the callback.', + `However, the preferred fix is to destructure the 'props' ` + + `object outside of the useEffect call and refer to specific ` + + `props directly by their names.`, ], }, { @@ -2529,7 +2639,9 @@ const tests = { errors: [ "React Hook useEffect has a missing dependency: 'props'. " + 'Either include it or remove the dependency array. ' + - 'Alternatively, destructure the necessary props outside the callback.', + `However, the preferred fix is to destructure the 'props' ` + + `object outside of the useEffect call and refer to specific ` + + `props directly by their names.`, ], }, { @@ -2556,7 +2668,9 @@ const tests = { errors: [ "React Hook useEffect has a missing dependency: 'props'. " + 'Either include it or remove the dependency array. ' + - 'Alternatively, destructure the necessary props outside the callback.', + `However, the preferred fix is to destructure the 'props' ` + + `object outside of the useEffect call and refer to specific ` + + `props directly by their names.`, ], }, { @@ -2583,7 +2697,9 @@ const tests = { errors: [ "React Hook useEffect has missing dependencies: 'props' and 'skillsCount'. " + 'Either include them or remove the dependency array. ' + - 'Alternatively, destructure the necessary props outside the callback.', + `However, the preferred fix is to destructure the 'props' ` + + `object outside of the useEffect call and refer to specific ` + + `props directly by their names.`, ], }, { @@ -2638,11 +2754,15 @@ const tests = { let value; let value2; let value3; + let value4; let asyncValue; useEffect(() => { - value = {}; + if (value4) { + value = {}; + } value2 = 100; value = 43; + value4 = true; console.log(value2); console.log(value3); setTimeout(() => { @@ -2659,11 +2779,15 @@ const tests = { let value; let value2; let value3; + let value4; let asyncValue; useEffect(() => { - value = {}; + if (value4) { + value = {}; + } value2 = 100; value = 43; + value4 = true; console.log(value2); console.log(value3); setTimeout(() => { @@ -2673,14 +2797,20 @@ const tests = { } `, errors: [ + // value2 + `Assignments to the 'value2' variable from inside a React useEffect Hook ` + + `will not persist between re-renders. ` + + `If it's only needed by this Hook, move the variable inside it. ` + + `Alternatively, declare a ref with the useRef Hook, ` + + `and keep the mutable value in its 'current' property.`, // value `Assignments to the 'value' variable from inside a React useEffect Hook ` + `will not persist between re-renders. ` + `If it's only needed by this Hook, move the variable inside it. ` + `Alternatively, declare a ref with the useRef Hook, ` + `and keep the mutable value in its 'current' property.`, - // value2 - `Assignments to the 'value2' variable from inside a React useEffect Hook ` + + // value4 + `Assignments to the 'value4' variable from inside a React useEffect Hook ` + `will not persist between re-renders. ` + `If it's only needed by this Hook, move the variable inside it. ` + `Alternatively, declare a ref with the useRef Hook, ` + @@ -3339,7 +3469,7 @@ const tests = { `The 'handleNext' function makes the dependencies of ` + `useEffect Hook (at line 11) change on every render. ` + `To fix this, move the 'handleNext' function ` + - `inside the useEffect callback. Alternatively, ` + + `inside the useEffect callback (at line 9). Alternatively, ` + `wrap the 'handleNext' definition into its own useCallback() Hook.`, ], }, @@ -3378,7 +3508,7 @@ const tests = { `The 'handleNext' function makes the dependencies of ` + `useEffect Hook (at line 11) change on every render. ` + `To fix this, move the 'handleNext' function ` + - `inside the useEffect callback. Alternatively, ` + + `inside the useEffect callback (at line 9). Alternatively, ` + `wrap the 'handleNext' definition into its own useCallback() Hook.`, ], }, @@ -3476,15 +3606,15 @@ const tests = { errors: [ "The 'handleNext1' function makes the dependencies of useEffect Hook " + "(at line 14) change on every render. To fix this, move the 'handleNext1' " + - 'function inside the useEffect callback. Alternatively, wrap the ' + + 'function inside the useEffect callback (at line 12). Alternatively, wrap the ' + "'handleNext1' definition into its own useCallback() Hook.", "The 'handleNext2' function makes the dependencies of useLayoutEffect Hook " + "(at line 17) change on every render. To fix this, move the 'handleNext2' " + - 'function inside the useLayoutEffect callback. Alternatively, wrap the ' + + 'function inside the useLayoutEffect callback (at line 15). Alternatively, wrap the ' + "'handleNext2' definition into its own useCallback() Hook.", "The 'handleNext3' function makes the dependencies of useMemo Hook " + "(at line 20) change on every render. To fix this, move the 'handleNext3' " + - 'function inside the useMemo callback. Alternatively, wrap the ' + + 'function inside the useMemo callback (at line 18). Alternatively, wrap the ' + "'handleNext3' definition into its own useCallback() Hook.", ], }, @@ -3544,15 +3674,15 @@ const tests = { errors: [ "The 'handleNext1' function makes the dependencies of useEffect Hook " + "(at line 15) change on every render. To fix this, move the 'handleNext1' " + - 'function inside the useEffect callback. Alternatively, wrap the ' + + 'function inside the useEffect callback (at line 12). Alternatively, wrap the ' + "'handleNext1' definition into its own useCallback() Hook.", "The 'handleNext2' function makes the dependencies of useLayoutEffect Hook " + "(at line 19) change on every render. To fix this, move the 'handleNext2' " + - 'function inside the useLayoutEffect callback. Alternatively, wrap the ' + + 'function inside the useLayoutEffect callback (at line 16). Alternatively, wrap the ' + "'handleNext2' definition into its own useCallback() Hook.", "The 'handleNext3' function makes the dependencies of useMemo Hook " + "(at line 23) change on every render. To fix this, move the 'handleNext3' " + - 'function inside the useMemo callback. Alternatively, wrap the ' + + 'function inside the useMemo callback (at line 20). Alternatively, wrap the ' + "'handleNext3' definition into its own useCallback() Hook.", ], }, @@ -3700,6 +3830,47 @@ const tests = { "'handleNext2' definition into its own useCallback() Hook.", ], }, + { + code: ` + function MyComponent(props) { + let handleNext = () => { + console.log('hello'); + }; + if (props.foo) { + handleNext = () => { + console.log('hello'); + }; + } + useEffect(() => { + return Store.subscribe(handleNext); + }, [handleNext]); + } + `, + // Normally we'd suggest moving handleNext inside an + // effect. But it's used more than once. + // TODO: our autofix here isn't quite sufficient because + // it only wraps the first definition. But seems ok. + output: ` + function MyComponent(props) { + let handleNext = useCallback(() => { + console.log('hello'); + }); + if (props.foo) { + handleNext = () => { + console.log('hello'); + }; + } + useEffect(() => { + return Store.subscribe(handleNext); + }, [handleNext]); + } + `, + errors: [ + "The 'handleNext' function makes the dependencies of useEffect Hook " + + '(at line 13) change on every render. To fix this, wrap the ' + + "'handleNext' definition into its own useCallback() Hook.", + ], + }, { code: ` function MyComponent(props) { @@ -3737,7 +3908,7 @@ const tests = { `The 'handleNext' function makes the dependencies of ` + `useEffect Hook (at line 14) change on every render. ` + `To fix this, move the 'handleNext' function inside ` + - `the useEffect callback. Alternatively, wrap the ` + + `the useEffect callback (at line 12). Alternatively, wrap the ` + `'handleNext' definition into its own useCallback() Hook.`, ], }, @@ -3770,13 +3941,260 @@ const tests = { return

{count}

; } `, - // TODO: ideally this should suggest useState updater form - // since this code doesn't actually work. errors: [ "React Hook useEffect has a missing dependency: 'count'. " + + 'Either include it or remove the dependency array. ' + + `If 'count' is only necessary for calculating the next state, ` + + `consider refactoring to the setCount(count => ...) form which ` + + `doesn't need to depend on the state from outside.`, + ], + }, + { + code: ` + function Counter() { + let [count, setCount] = useState(0); + let [increment, setIncrement] = useState(0); + + useEffect(() => { + let id = setInterval(() => { + setCount(count + increment); + }, 1000); + return () => clearInterval(id); + }, []); + + return

{count}

; + } + `, + output: ` + function Counter() { + let [count, setCount] = useState(0); + let [increment, setIncrement] = useState(0); + + useEffect(() => { + let id = setInterval(() => { + setCount(count + increment); + }, 1000); + return () => clearInterval(id); + }, [count, increment]); + + return

{count}

; + } + `, + errors: [ + "React Hook useEffect has missing dependencies: 'count' and 'increment'. " + + 'Either include them or remove the dependency array. ' + + `If 'count' is only necessary for calculating the next state, ` + + `consider refactoring to the setCount(count => ...) form which ` + + `doesn't need to depend on the state from outside.`, + ], + }, + { + code: ` + function Counter() { + let [count, setCount] = useState(0); + let [increment, setIncrement] = useState(0); + + useEffect(() => { + let id = setInterval(() => { + setCount(count => count + increment); + }, 1000); + return () => clearInterval(id); + }, []); + + return

{count}

; + } + `, + output: ` + function Counter() { + let [count, setCount] = useState(0); + let [increment, setIncrement] = useState(0); + + useEffect(() => { + let id = setInterval(() => { + setCount(count => count + increment); + }, 1000); + return () => clearInterval(id); + }, [increment]); + + return

{count}

; + } + `, + errors: [ + "React Hook useEffect has a missing dependency: 'increment'. " + + 'Either include it or remove the dependency array. ' + + `If 'increment' is only necessary for calculating the next state, ` + + `consider refactoring to the useReducer Hook. This ` + + `lets you move the calculation of next state outside the effect.`, + ], + }, + { + code: ` + function Counter() { + let [count, setCount] = useState(0); + let increment = useCustomHook(); + + useEffect(() => { + let id = setInterval(() => { + setCount(count => count + increment); + }, 1000); + return () => clearInterval(id); + }, []); + + return

{count}

; + } + `, + output: ` + function Counter() { + let [count, setCount] = useState(0); + let increment = useCustomHook(); + + useEffect(() => { + let id = setInterval(() => { + setCount(count => count + increment); + }, 1000); + return () => clearInterval(id); + }, [increment]); + + return

{count}

; + } + `, + // This intentionally doesn't show the reducer message + // because we don't know if it's safe for it to close over a value. + // We only show it for state variables (and possibly props). + errors: [ + "React Hook useEffect has a missing dependency: 'increment'. " + + 'Either include it or remove the dependency array.', + ], + }, + { + code: ` + function Counter({ step }) { + let [count, setCount] = useState(0); + + function increment(x) { + return x + step; + } + + useEffect(() => { + let id = setInterval(() => { + setCount(count => increment(count)); + }, 1000); + return () => clearInterval(id); + }, []); + + return

{count}

; + } + `, + output: ` + function Counter({ step }) { + let [count, setCount] = useState(0); + + function increment(x) { + return x + step; + } + + useEffect(() => { + let id = setInterval(() => { + setCount(count => increment(count)); + }, 1000); + return () => clearInterval(id); + }, [increment]); + + return

{count}

; + } + `, + // This intentionally doesn't show the reducer message + // because we don't know if it's safe for it to close over a value. + // We only show it for state variables (and possibly props). + errors: [ + "React Hook useEffect has a missing dependency: 'increment'. " + 'Either include it or remove the dependency array.', ], }, + { + code: ` + function Counter({ step }) { + let [count, setCount] = useState(0); + + function increment(x) { + return x + step; + } + + useEffect(() => { + let id = setInterval(() => { + setCount(count => increment(count)); + }, 1000); + return () => clearInterval(id); + }, [increment]); + + return

{count}

; + } + `, + output: ` + function Counter({ step }) { + let [count, setCount] = useState(0); + + function increment(x) { + return x + step; + } + + useEffect(() => { + let id = setInterval(() => { + setCount(count => increment(count)); + }, 1000); + return () => clearInterval(id); + }, [increment]); + + return

{count}

; + } + `, + errors: [ + `The 'increment' function makes the dependencies of useEffect Hook ` + + `(at line 14) change on every render. To fix this, move the ` + + `'increment' function inside the useEffect callback (at line 9). ` + + `Alternatively, wrap the \'increment\' definition into its own ` + + `useCallback() Hook.`, + ], + }, + { + code: ` + function Counter({ increment }) { + let [count, setCount] = useState(0); + + useEffect(() => { + let id = setInterval(() => { + setCount(count => count + increment); + }, 1000); + return () => clearInterval(id); + }, []); + + return

{count}

; + } + `, + output: ` + function Counter({ increment }) { + let [count, setCount] = useState(0); + + useEffect(() => { + let id = setInterval(() => { + setCount(count => count + increment); + }, 1000); + return () => clearInterval(id); + }, [increment]); + + return

{count}

; + } + `, + errors: [ + "React Hook useEffect has a missing dependency: 'increment'. " + + 'Either include it or remove the dependency array. ' + + `If 'increment' is only necessary for calculating the next state, ` + + `consider refactoring to the useReducer Hook. This lets you move ` + + `the calculation of next state outside the effect. ` + + `You can then read 'increment' from the reducer ` + + `by putting it directly in your component.`, + ], + }, { code: ` function Counter() { @@ -3849,6 +4267,112 @@ const tests = { `Either include it or remove the dependency array.`, ], }, + { + code: ` + function Podcasts({ fetchPodcasts, id }) { + let [podcasts, setPodcasts] = useState(null); + useEffect(() => { + fetchPodcasts(id).then(setPodcasts); + }, [id]); + } + `, + output: ` + function Podcasts({ fetchPodcasts, id }) { + let [podcasts, setPodcasts] = useState(null); + useEffect(() => { + fetchPodcasts(id).then(setPodcasts); + }, [fetchPodcasts, id]); + } + `, + errors: [ + `React Hook useEffect has a missing dependency: 'fetchPodcasts'. ` + + `Either include it or remove the dependency array. ` + + `If specifying 'fetchPodcasts' makes the dependencies change too often, ` + + `find the parent component that defines it and wrap that definition in useCallback.`, + ], + }, + { + code: ` + function Podcasts({ api: { fetchPodcasts }, id }) { + let [podcasts, setPodcasts] = useState(null); + useEffect(() => { + fetchPodcasts(id).then(setPodcasts); + }, [id]); + } + `, + output: ` + function Podcasts({ api: { fetchPodcasts }, id }) { + let [podcasts, setPodcasts] = useState(null); + useEffect(() => { + fetchPodcasts(id).then(setPodcasts); + }, [fetchPodcasts, id]); + } + `, + errors: [ + `React Hook useEffect has a missing dependency: 'fetchPodcasts'. ` + + `Either include it or remove the dependency array. ` + + `If specifying 'fetchPodcasts' makes the dependencies change too often, ` + + `find the parent component that defines it and wrap that definition in useCallback.`, + ], + }, + { + code: ` + function Podcasts({ fetchPodcasts, fetchPodcasts2, id }) { + let [podcasts, setPodcasts] = useState(null); + useEffect(() => { + setTimeout(() => { + console.log(id); + fetchPodcasts(id).then(setPodcasts); + fetchPodcasts2(id).then(setPodcasts); + }); + }, [id]); + } + `, + output: ` + function Podcasts({ fetchPodcasts, fetchPodcasts2, id }) { + let [podcasts, setPodcasts] = useState(null); + useEffect(() => { + setTimeout(() => { + console.log(id); + fetchPodcasts(id).then(setPodcasts); + fetchPodcasts2(id).then(setPodcasts); + }); + }, [fetchPodcasts, fetchPodcasts2, id]); + } + `, + errors: [ + `React Hook useEffect has missing dependencies: 'fetchPodcasts' and 'fetchPodcasts2'. ` + + `Either include them or remove the dependency array. ` + + `If specifying 'fetchPodcasts' makes the dependencies change too often, ` + + `find the parent component that defines it and wrap that definition in useCallback.`, + ], + }, + { + code: ` + function Podcasts({ fetchPodcasts, id }) { + let [podcasts, setPodcasts] = useState(null); + useEffect(() => { + console.log(fetchPodcasts); + fetchPodcasts(id).then(setPodcasts); + }, [id]); + } + `, + output: ` + function Podcasts({ fetchPodcasts, id }) { + let [podcasts, setPodcasts] = useState(null); + useEffect(() => { + console.log(fetchPodcasts); + fetchPodcasts(id).then(setPodcasts); + }, [fetchPodcasts, id]); + } + `, + errors: [ + `React Hook useEffect has a missing dependency: 'fetchPodcasts'. ` + + `Either include it or remove the dependency array. ` + + `If specifying 'fetchPodcasts' makes the dependencies change too often, ` + + `find the parent component that defines it and wrap that definition in useCallback.`, + ], + }, ], }; diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index 2f8b18c0069a7..512f71db07500 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -35,6 +35,8 @@ export default { const options = {additionalHooks}; // Should be shared between visitors. + let setStateCallSites = new WeakMap(); + let stateVariables = new WeakSet(); let staticKnownValueCache = new WeakMap(); let functionWithoutCapturedValueCache = new WeakMap(); function memoizeWithWeakMap(fn, map) { @@ -204,19 +206,40 @@ export default { return false; } const id = def.node.id; - if (callee.name === 'useRef' && id.type === 'Identifier') { + const {name} = callee; + if (name === 'useRef' && id.type === 'Identifier') { // useRef() return value is static. return true; - } else if (callee.name === 'useState' || callee.name === 'useReducer') { + } else if (name === 'useState' || name === 'useReducer') { // Only consider second value in initializing tuple static. if ( id.type === 'ArrayPattern' && id.elements.length === 2 && - Array.isArray(resolved.identifiers) && - // Is second tuple value the same reference we're checking? - id.elements[1] === resolved.identifiers[0] + Array.isArray(resolved.identifiers) ) { - return true; + // Is second tuple value the same reference we're checking? + if (id.elements[1] === resolved.identifiers[0]) { + if (name === 'useState') { + const references = resolved.references; + for (let i = 0; i < references.length; i++) { + setStateCallSites.set( + references[i].identifier, + id.elements[0], + ); + } + } + // Setter is static. + return true; + } else if (id.elements[0] === resolved.identifiers[0]) { + if (name === 'useState') { + const references = resolved.references; + for (let i = 0; i < references.length; i++) { + stateVariables.add(references[i].identifier); + } + } + // State variable itself is dynamic. + return false; + } } } // By default assume it's dynamic. @@ -365,8 +388,10 @@ export default { memoizedIsFunctionWithoutCapturedValues(resolved); dependencies.set(dependency, { isStatic, - reference, + references: [reference], }); + } else { + dependencies.get(dependency).references.push(reference); } } for (const childScope of currentScope.childScopes) { @@ -514,9 +539,12 @@ export default { // Warn about assigning to variables in the outer scope. // Those are usually bugs. - let foundStaleAssignments = false; + let staleAssignments = new Set(); function reportStaleAssignment(writeExpr, key) { - foundStaleAssignments = true; + if (staleAssignments.has(key)) { + return; + } + staleAssignments.add(key); context.report({ node: writeExpr, message: @@ -532,15 +560,17 @@ export default { // Remember which deps are optional and report bad usage first. const optionalDependencies = new Set(); - dependencies.forEach(({isStatic, reference}, key) => { + dependencies.forEach(({isStatic, references}, key) => { if (isStatic) { optionalDependencies.add(key); } - if (reference.writeExpr) { - reportStaleAssignment(reference.writeExpr, key); - } + references.forEach(reference => { + if (reference.writeExpr) { + reportStaleAssignment(reference.writeExpr, key); + } + }); }); - if (foundStaleAssignments) { + if (staleAssignments.size > 0) { // The intent isn't clear so we'll wait until you fix those first. return; } @@ -588,8 +618,10 @@ export default { } else { message += ` To fix this, move the '${fn.name.name}' function ` + - `inside the ${reactiveHookName} callback. Alternatively, ` + - `wrap the '${ + `inside the ${reactiveHookName} callback (at line ${ + node.loc.start.line + }). ` + + `Alternatively, wrap the '${ fn.name.name }' definition into its own useCallback() Hook.`; } @@ -697,7 +729,7 @@ export default { if (propDep == null) { return; } - const refs = propDep.reference.resolved.references; + const refs = propDep.references; if (!Array.isArray(refs)) { return; } @@ -724,8 +756,154 @@ export default { } if (isPropsOnlyUsedInMembers) { extraWarning = - ' Alternatively, destructure the necessary props ' + - 'outside the callback.'; + ` However, the preferred fix is to destructure the 'props' ` + + `object outside of the ${reactiveHookName} call and ` + + `refer to specific props directly by their names.`; + } + } + + if (!extraWarning && missingDependencies.size > 0) { + // See if the user is trying to avoid specifying a callable prop. + // This usually means they're unaware of useCallback. + let missingCallbackDep = null; + missingDependencies.forEach(missingDep => { + if (missingCallbackDep) { + return; + } + // Is this a variable from top scope? + const topScopeRef = componentScope.set.get(missingDep); + const usedDep = dependencies.get(missingDep); + if (usedDep.references[0].resolved !== topScopeRef) { + return; + } + // Is this a destructured prop? + const def = topScopeRef.defs[0]; + if (def == null || def.name == null || def.type !== 'Parameter') { + return; + } + // Was it called in at least one case? Then it's a function. + let isFunctionCall = false; + let id; + for (let i = 0; i < usedDep.references.length; i++) { + id = usedDep.references[i].identifier; + if ( + id != null && + id.parent != null && + id.parent.type === 'CallExpression' && + id.parent.callee === id + ) { + isFunctionCall = true; + break; + } + } + if (!isFunctionCall) { + return; + } + // If it's missing (i.e. in component scope) *and* it's a parameter + // then it is definitely coming from props destructuring. + // (It could also be props itself but we wouldn't be calling it then.) + missingCallbackDep = missingDep; + }); + if (missingCallbackDep !== null) { + extraWarning = + ` If specifying '${missingCallbackDep}'` + + ` makes the dependencies change too often, ` + + `find the parent component that defines it ` + + `and wrap that definition in useCallback.`; + } + } + + if (!extraWarning && missingDependencies.size > 0) { + let setStateRecommendation = null; + missingDependencies.forEach(missingDep => { + if (setStateRecommendation !== null) { + return; + } + const usedDep = dependencies.get(missingDep); + const references = usedDep.references; + let id; + let maybeCall; + for (let i = 0; i < references.length; i++) { + id = references[i].identifier; + maybeCall = id.parent; + // Try to see if we have setState(someExpr(missingDep)). + while (maybeCall != null && maybeCall !== componentScope.block) { + if (maybeCall.type === 'CallExpression') { + const correspondingStateVariable = setStateCallSites.get( + maybeCall.callee, + ); + if (correspondingStateVariable != null) { + if (correspondingStateVariable.name === missingDep) { + // setCount(count + 1) + setStateRecommendation = { + missingDep, + setter: maybeCall.callee.name, + form: 'updater', + }; + } else if (stateVariables.has(id)) { + // setCount(count + increment) + setStateRecommendation = { + missingDep, + setter: maybeCall.callee.name, + form: 'reducer', + }; + } else { + const resolved = references[i].resolved; + if (resolved != null) { + // If it's a parameter *and* a missing dep, + // it must be a prop or something inside a prop. + // Therefore, recommend an inline reducer. + const def = resolved.defs[0]; + if (def != null && def.type === 'Parameter') { + setStateRecommendation = { + missingDep, + setter: maybeCall.callee.name, + form: 'inlineReducer', + }; + } + } + } + break; + } + } + maybeCall = maybeCall.parent; + } + if (setStateRecommendation !== null) { + break; + } + } + }); + if (setStateRecommendation !== null) { + let suggestion; + switch (setStateRecommendation.form) { + case 'reducer': + suggestion = + 'useReducer Hook. This lets you move the calculation ' + + 'of next state outside the effect.'; + break; + case 'inlineReducer': + suggestion = + 'useReducer Hook. This lets you move the calculation ' + + 'of next state outside the effect. You can then ' + + `read '${ + setStateRecommendation.missingDep + }' from the reducer ` + + `by putting it directly in your component.`; + break; + case 'updater': + suggestion = + `${setStateRecommendation.setter}(${ + setStateRecommendation.missingDep + } => ...) form ` + + `which doesn't need to depend on the state from outside.`; + break; + default: + throw new Error('Unknown case.'); + } + extraWarning = + ` If '${setStateRecommendation.missingDep}'` + + ` is only necessary for calculating the next state, ` + + `consider refactoring to the ${suggestion}`; } } @@ -981,9 +1159,7 @@ function scanForDeclaredBareFunctions({ if (currentScope !== scope) { // This reference is outside the Hook callback. // It can only be legit if it's the deps array. - if (isAncestorNodeOf(declaredDependenciesNode, reference.identifier)) { - continue; - } else { + if (!isAncestorNodeOf(declaredDependenciesNode, reference.identifier)) { return true; } } From eb6247a9ab7653aac346db08faf0862c58b055df Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 7 Mar 2019 15:21:44 +0000 Subject: [PATCH 09/22] More concise messages (#15053) --- .../ESLintRuleExhaustiveDeps-test.js | 112 +++++++++--------- .../src/ExhaustiveDeps.js | 47 ++++---- 2 files changed, 80 insertions(+), 79 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index c30146cac3635..dfb8a67856bf0 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -1236,7 +1236,7 @@ const tests = { errors: [ "React Hook useCallback has a missing dependency: 'local2'. " + 'Either include it or remove the dependency array. ' + - "Values like 'local1' aren't valid dependencies " + + "Outer scope values like 'local1' aren't valid dependencies " + "because their mutation doesn't re-render the component.", ], }, @@ -1302,7 +1302,7 @@ const tests = { errors: [ "React Hook useCallback has an unnecessary dependency: 'window'. " + 'Either exclude it or remove the dependency array. ' + - "Values like 'window' aren't valid dependencies " + + "Outer scope values like 'window' aren't valid dependencies " + "because their mutation doesn't re-render the component.", ], }, @@ -2304,9 +2304,8 @@ const tests = { errors: [ "React Hook useEffect has a missing dependency: 'state'. " + 'Either include it or remove the dependency array. ' + - `If 'state' is only necessary for calculating the next state, ` + - `consider refactoring to the setState(state => ...) form which ` + - `doesn't need to depend on the state from outside.`, + `You can also write 'setState(state => ...)' ` + + `if you only use 'state' for the 'setState' call.`, ], }, { @@ -2336,9 +2335,8 @@ const tests = { errors: [ "React Hook useEffect has a missing dependency: 'state'. " + 'Either include it or remove the dependency array. ' + - `If 'state' is only necessary for calculating the next state, ` + - `consider refactoring to the setState(state => ...) form which ` + - `doesn't need to depend on the state from outside.`, + `You can also write 'setState(state => ...)' ` + + `if you only use 'state' for the 'setState' call.`, ], }, { @@ -3066,7 +3064,7 @@ const tests = { errors: [ "React Hook useEffect has an unnecessary dependency: 'window'. " + 'Either exclude it or remove the dependency array. ' + - "Values like 'window' aren't valid dependencies " + + "Outer scope values like 'window' aren't valid dependencies " + "because their mutation doesn't re-render the component.", ], }, @@ -3092,7 +3090,7 @@ const tests = { errors: [ "React Hook useEffect has an unnecessary dependency: 'MutableStore.hello'. " + 'Either exclude it or remove the dependency array. ' + - "Values like 'MutableStore.hello' aren't valid dependencies " + + "Outer scope values like 'MutableStore.hello' aren't valid dependencies " + "because their mutation doesn't re-render the component.", ], }, @@ -3129,7 +3127,7 @@ const tests = { 'React Hook useEffect has unnecessary dependencies: ' + "'MutableStore.hello.world', 'global.stuff', and 'z'. " + 'Either exclude them or remove the dependency array. ' + - "Values like 'MutableStore.hello.world' aren't valid dependencies " + + "Outer scope values like 'MutableStore.hello.world' aren't valid dependencies " + "because their mutation doesn't re-render the component.", ], }, @@ -3168,7 +3166,7 @@ const tests = { 'React Hook useEffect has unnecessary dependencies: ' + "'MutableStore.hello.world', 'global.stuff', and 'z'. " + 'Either exclude them or remove the dependency array. ' + - "Values like 'MutableStore.hello.world' aren't valid dependencies " + + "Outer scope values like 'MutableStore.hello.world' aren't valid dependencies " + "because their mutation doesn't re-render the component.", ], }, @@ -3205,7 +3203,7 @@ const tests = { 'React Hook useCallback has unnecessary dependencies: ' + "'MutableStore.hello.world', 'global.stuff', 'props.foo', 'x', 'y', and 'z'. " + 'Either exclude them or remove the dependency array. ' + - "Values like 'MutableStore.hello.world' aren't valid dependencies " + + "Outer scope values like 'MutableStore.hello.world' aren't valid dependencies " + "because their mutation doesn't re-render the component.", ], }, @@ -3468,8 +3466,7 @@ const tests = { errors: [ `The 'handleNext' function makes the dependencies of ` + `useEffect Hook (at line 11) change on every render. ` + - `To fix this, move the 'handleNext' function ` + - `inside the useEffect callback (at line 9). Alternatively, ` + + `Move it inside the useEffect callback. Alternatively, ` + `wrap the 'handleNext' definition into its own useCallback() Hook.`, ], }, @@ -3507,8 +3504,7 @@ const tests = { errors: [ `The 'handleNext' function makes the dependencies of ` + `useEffect Hook (at line 11) change on every render. ` + - `To fix this, move the 'handleNext' function ` + - `inside the useEffect callback (at line 9). Alternatively, ` + + `Move it inside the useEffect callback. Alternatively, ` + `wrap the 'handleNext' definition into its own useCallback() Hook.`, ], }, @@ -3605,17 +3601,14 @@ const tests = { `, errors: [ "The 'handleNext1' function makes the dependencies of useEffect Hook " + - "(at line 14) change on every render. To fix this, move the 'handleNext1' " + - 'function inside the useEffect callback (at line 12). Alternatively, wrap the ' + - "'handleNext1' definition into its own useCallback() Hook.", + '(at line 14) change on every render. Move it inside the useEffect callback. ' + + "Alternatively, wrap the 'handleNext1' definition into its own useCallback() Hook.", "The 'handleNext2' function makes the dependencies of useLayoutEffect Hook " + - "(at line 17) change on every render. To fix this, move the 'handleNext2' " + - 'function inside the useLayoutEffect callback (at line 15). Alternatively, wrap the ' + - "'handleNext2' definition into its own useCallback() Hook.", + '(at line 17) change on every render. Move it inside the useLayoutEffect callback. ' + + "Alternatively, wrap the 'handleNext2' definition into its own useCallback() Hook.", "The 'handleNext3' function makes the dependencies of useMemo Hook " + - "(at line 20) change on every render. To fix this, move the 'handleNext3' " + - 'function inside the useMemo callback (at line 18). Alternatively, wrap the ' + - "'handleNext3' definition into its own useCallback() Hook.", + '(at line 20) change on every render. Move it inside the useMemo callback. ' + + "Alternatively, wrap the 'handleNext3' definition into its own useCallback() Hook.", ], }, { @@ -3673,17 +3666,14 @@ const tests = { `, errors: [ "The 'handleNext1' function makes the dependencies of useEffect Hook " + - "(at line 15) change on every render. To fix this, move the 'handleNext1' " + - 'function inside the useEffect callback (at line 12). Alternatively, wrap the ' + - "'handleNext1' definition into its own useCallback() Hook.", + '(at line 15) change on every render. Move it inside the useEffect callback. ' + + "Alternatively, wrap the 'handleNext1' definition into its own useCallback() Hook.", "The 'handleNext2' function makes the dependencies of useLayoutEffect Hook " + - "(at line 19) change on every render. To fix this, move the 'handleNext2' " + - 'function inside the useLayoutEffect callback (at line 16). Alternatively, wrap the ' + - "'handleNext2' definition into its own useCallback() Hook.", + '(at line 19) change on every render. Move it inside the useLayoutEffect callback. ' + + "Alternatively, wrap the 'handleNext2' definition into its own useCallback() Hook.", "The 'handleNext3' function makes the dependencies of useMemo Hook " + - "(at line 23) change on every render. To fix this, move the 'handleNext3' " + - 'function inside the useMemo callback (at line 20). Alternatively, wrap the ' + - "'handleNext3' definition into its own useCallback() Hook.", + '(at line 23) change on every render. Move it inside the useMemo callback. ' + + "Alternatively, wrap the 'handleNext3' definition into its own useCallback() Hook.", ], }, { @@ -3907,8 +3897,7 @@ const tests = { errors: [ `The 'handleNext' function makes the dependencies of ` + `useEffect Hook (at line 14) change on every render. ` + - `To fix this, move the 'handleNext' function inside ` + - `the useEffect callback (at line 12). Alternatively, wrap the ` + + `Move it inside the useEffect callback. Alternatively, wrap the ` + `'handleNext' definition into its own useCallback() Hook.`, ], }, @@ -3944,9 +3933,8 @@ const tests = { errors: [ "React Hook useEffect has a missing dependency: 'count'. " + 'Either include it or remove the dependency array. ' + - `If 'count' is only necessary for calculating the next state, ` + - `consider refactoring to the setCount(count => ...) form which ` + - `doesn't need to depend on the state from outside.`, + `You can also write 'setCount(count => ...)' if you ` + + `only use 'count' for the 'setCount' call.`, ], }, { @@ -3983,9 +3971,8 @@ const tests = { errors: [ "React Hook useEffect has missing dependencies: 'count' and 'increment'. " + 'Either include them or remove the dependency array. ' + - `If 'count' is only necessary for calculating the next state, ` + - `consider refactoring to the setCount(count => ...) form which ` + - `doesn't need to depend on the state from outside.`, + `You can also write 'setCount(count => ...)' if you ` + + `only use 'count' for the 'setCount' call.`, ], }, { @@ -4022,9 +4009,8 @@ const tests = { errors: [ "React Hook useEffect has a missing dependency: 'increment'. " + 'Either include it or remove the dependency array. ' + - `If 'increment' is only necessary for calculating the next state, ` + - `consider refactoring to the useReducer Hook. This ` + - `lets you move the calculation of next state outside the effect.`, + `You can also replace multiple useState variables with useReducer ` + + `if 'setCount' needs the current value of 'increment'.`, ], }, { @@ -4150,8 +4136,7 @@ const tests = { `, errors: [ `The 'increment' function makes the dependencies of useEffect Hook ` + - `(at line 14) change on every render. To fix this, move the ` + - `'increment' function inside the useEffect callback (at line 9). ` + + `(at line 14) change on every render. Move it inside the useEffect callback. ` + `Alternatively, wrap the \'increment\' definition into its own ` + `useCallback() Hook.`, ], @@ -4188,11 +4173,8 @@ const tests = { errors: [ "React Hook useEffect has a missing dependency: 'increment'. " + 'Either include it or remove the dependency array. ' + - `If 'increment' is only necessary for calculating the next state, ` + - `consider refactoring to the useReducer Hook. This lets you move ` + - `the calculation of next state outside the effect. ` + - `You can then read 'increment' from the reducer ` + - `by putting it directly in your component.`, + 'You can also replace useState with an inline useReducer ' + + `if 'setCount' needs the current value of 'increment'.`, ], }, { @@ -4373,6 +4355,30 @@ const tests = { `find the parent component that defines it and wrap that definition in useCallback.`, ], }, + { + // The mistake here is that it was moved inside the effect + // so it can't be referenced in the deps array. + code: ` + function Thing() { + useEffect(() => { + const fetchData = async () => {}; + fetchData(); + }, [fetchData]); + } + `, + output: ` + function Thing() { + useEffect(() => { + const fetchData = async () => {}; + fetchData(); + }, []); + } + `, + errors: [ + `React Hook useEffect has an unnecessary dependency: 'fetchData'. ` + + `Either exclude it or remove the dependency array.`, + ], + }, ], }; diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index 512f71db07500..c2e2d2f22bd74 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -617,10 +617,7 @@ export default { }' definition into its own useCallback() Hook.`; } else { message += - ` To fix this, move the '${fn.name.name}' function ` + - `inside the ${reactiveHookName} callback (at line ${ - node.loc.start.line - }). ` + + ` Move it inside the ${reactiveHookName} callback. ` + `Alternatively, wrap the '${ fn.name.name }' definition into its own useCallback() Hook.`; @@ -715,9 +712,13 @@ export default { "because their mutation doesn't re-render the component."; } else if (externalDependencies.size > 0) { const dep = Array.from(externalDependencies)[0]; - extraWarning = - ` Values like '${dep}' aren't valid dependencies ` + - `because their mutation doesn't re-render the component.`; + // Don't show this warning for things that likely just got moved *inside* the callback + // because in that case they're clearly not referring to globals. + if (!scope.set.has(dep)) { + extraWarning = + ` Outer scope values like '${dep}' aren't valid dependencies ` + + `because their mutation doesn't re-render the component.`; + } } } @@ -874,36 +875,30 @@ export default { } }); if (setStateRecommendation !== null) { - let suggestion; switch (setStateRecommendation.form) { case 'reducer': - suggestion = - 'useReducer Hook. This lets you move the calculation ' + - 'of next state outside the effect.'; + extraWarning = + ` You can also replace multiple useState variables with useReducer ` + + `if '${setStateRecommendation.setter}' needs the ` + + `current value of '${setStateRecommendation.missingDep}'.`; break; case 'inlineReducer': - suggestion = - 'useReducer Hook. This lets you move the calculation ' + - 'of next state outside the effect. You can then ' + - `read '${ - setStateRecommendation.missingDep - }' from the reducer ` + - `by putting it directly in your component.`; + extraWarning = + ` You can also replace useState with an inline useReducer ` + + `if '${setStateRecommendation.setter}' needs the ` + + `current value of '${setStateRecommendation.missingDep}'.`; break; case 'updater': - suggestion = - `${setStateRecommendation.setter}(${ + extraWarning = + ` You can also write '${setStateRecommendation.setter}(${ setStateRecommendation.missingDep - } => ...) form ` + - `which doesn't need to depend on the state from outside.`; + } => ...)' if you only use '${ + setStateRecommendation.missingDep + }'` + ` for the '${setStateRecommendation.setter}' call.`; break; default: throw new Error('Unknown case.'); } - extraWarning = - ` If '${setStateRecommendation.missingDep}'` + - ` is only necessary for calculating the next state, ` + - `consider refactoring to the ${suggestion}`; } } From 03ad9c73e468cafa9cafbd9a51a0c3a16ed3f362 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 7 Mar 2019 19:40:23 +0000 Subject: [PATCH 10/22] [ESLint] Tweak setState updater message and add useEffect(async) warning (#15055) * Use first letter in setCount(c => ...) suggestion In-person testing showed using original variable name confuses people. * Warn about async effects --- .../ESLintRuleExhaustiveDeps-test.js | 60 +++++++++++++++++-- .../src/ExhaustiveDeps.js | 31 +++++++++- 2 files changed, 84 insertions(+), 7 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index dfb8a67856bf0..337bcfabc42c4 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -949,6 +949,29 @@ const tests = { } `, }, + { + code: ` + function App() { + const [query, setQuery] = useState('react'); + const [state, setState] = useState(null); + useEffect(() => { + let ignore = false; + fetchSomething(); + async function fetchSomething() { + const result = await (await fetch('http://hn.algolia.com/api/v1/search?query=' + query)).json(); + if (!ignore) setState(result); + } + return () => { ignore = true; }; + }, [query]); + return ( + <> + setQuery(e.target.value)} /> + {JSON.stringify(state)} + + ); + } + `, + }, ], invalid: [ { @@ -2304,7 +2327,7 @@ const tests = { errors: [ "React Hook useEffect has a missing dependency: 'state'. " + 'Either include it or remove the dependency array. ' + - `You can also write 'setState(state => ...)' ` + + `You can also write 'setState(s => ...)' ` + `if you only use 'state' for the 'setState' call.`, ], }, @@ -2335,7 +2358,7 @@ const tests = { errors: [ "React Hook useEffect has a missing dependency: 'state'. " + 'Either include it or remove the dependency array. ' + - `You can also write 'setState(state => ...)' ` + + `You can also write 'setState(s => ...)' ` + `if you only use 'state' for the 'setState' call.`, ], }, @@ -3933,7 +3956,7 @@ const tests = { errors: [ "React Hook useEffect has a missing dependency: 'count'. " + 'Either include it or remove the dependency array. ' + - `You can also write 'setCount(count => ...)' if you ` + + `You can also write 'setCount(c => ...)' if you ` + `only use 'count' for the 'setCount' call.`, ], }, @@ -3971,7 +3994,7 @@ const tests = { errors: [ "React Hook useEffect has missing dependencies: 'count' and 'increment'. " + 'Either include them or remove the dependency array. ' + - `You can also write 'setCount(count => ...)' if you ` + + `You can also write 'setCount(c => ...)' if you ` + `only use 'count' for the 'setCount' call.`, ], }, @@ -4379,6 +4402,35 @@ const tests = { `Either exclude it or remove the dependency array.`, ], }, + { + code: ` + function Thing() { + useEffect(async () => {}, []); + } + `, + output: ` + function Thing() { + useEffect(async () => {}, []); + } + `, + errors: [ + `Effect callbacks are synchronous to prevent race conditions. ` + + `Put the async function inside:\n\n` + + `useEffect(() => {\n` + + ` let ignore = false;\n` + + ` fetchSomething();\n` + + `\n` + + ` async function fetchSomething() {\n` + + ` const result = await ...\n` + + ` if (!ignore) setState(result);\n` + + ` }\n` + + `\n` + + ` return () => { ignore = true; };\n` + + `}, []);\n` + + `\n` + + `This lets you handle multiple requests without bugs.`, + ], + }, ], }; diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index c2e2d2f22bd74..61a96e3431e4f 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -106,6 +106,28 @@ export default { return; } + if (isEffect && node.async) { + context.report({ + node: node, + message: + `Effect callbacks are synchronous to prevent race conditions. ` + + `Put the async function inside:\n\n` + + `useEffect(() => {\n` + + ` let ignore = false;\n` + + ` fetchSomething();\n` + + `\n` + + ` async function fetchSomething() {\n` + + ` const result = await ...\n` + + ` if (!ignore) setState(result);\n` + + ` }\n` + + `\n` + + ` return () => { ignore = true; };\n` + + `}, []);\n` + + `\n` + + `This lets you handle multiple requests without bugs.`, + }); + } + // Get the current scope. const scope = context.getScope(); @@ -890,9 +912,12 @@ export default { break; case 'updater': extraWarning = - ` You can also write '${setStateRecommendation.setter}(${ - setStateRecommendation.missingDep - } => ...)' if you only use '${ + ` You can also write '${ + setStateRecommendation.setter + }(${setStateRecommendation.missingDep.substring( + 0, + 1, + )} => ...)' if you only use '${ setStateRecommendation.missingDep }'` + ` for the '${setStateRecommendation.setter}' call.`; break; From d0289c7e3a2dfc349dcce7f9eb3dee22464e97bd Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 7 Mar 2019 19:43:07 +0000 Subject: [PATCH 11/22] eslint-plugin-react-hooks@1.5.0 --- packages/eslint-plugin-react-hooks/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/eslint-plugin-react-hooks/package.json b/packages/eslint-plugin-react-hooks/package.json index 348ac76aa5b94..5f1e754f88f6d 100644 --- a/packages/eslint-plugin-react-hooks/package.json +++ b/packages/eslint-plugin-react-hooks/package.json @@ -1,7 +1,7 @@ { "name": "eslint-plugin-react-hooks", "description": "ESLint rules for React Hooks", - "version": "1.4.0", + "version": "1.5.0", "repository": { "type": "git", "url": "https://github.com/facebook/react.git", From 3f4852fa5f932e5e815ef1be39c72ac8be4687d4 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Fri, 8 Mar 2019 18:53:14 -0800 Subject: [PATCH 12/22] Run Placeholder tests in persistent mode, too (#15013) * Convert ReactSuspensePlaceholder tests to use noop Instead of the test renderer, since test renderer does not support running in persistent mode. * Run Placeholder tests in persistent mode, too * Fix Flow and lint * Hidden text instances should have correct host context Adds a test for a subtle edge case that only occurs in persistent mode. * createHiddenTextInstance -> cloneHiddenTextInstance This sidesteps the problem where createHiddenTextInstance needs access to the host context. --- .../src/ReactFabricHostConfig.js | 13 +- .../src/createReactNoop.js | 112 +++++++++-- .../src/ReactFiberCommitWork.js | 98 +++++----- .../src/ReactFiberCompleteWork.js | 83 +++----- .../ReactSuspensePlaceholder-test.internal.js | 182 +++++++++++++----- .../src/forks/ReactFiberHostConfig.custom.js | 4 +- .../shared/HostConfigWithNoPersistence.js | 3 +- 7 files changed, 328 insertions(+), 167 deletions(-) diff --git a/packages/react-native-renderer/src/ReactFabricHostConfig.js b/packages/react-native-renderer/src/ReactFabricHostConfig.js index bb4646b190bce..0eed32d066246 100644 --- a/packages/react-native-renderer/src/ReactFabricHostConfig.js +++ b/packages/react-native-renderer/src/ReactFabricHostConfig.js @@ -406,10 +406,17 @@ export function cloneUnhiddenInstance( }; } -export function createHiddenTextInstance( +export function cloneHiddenTextInstance( + instance: Instance, + text: string, + internalInstanceHandle: Object, +): TextInstance { + throw new Error('Not yet implemented.'); +} + +export function cloneUnhiddenTextInstance( + instance: Instance, text: string, - rootContainerInstance: Container, - hostContext: HostContext, internalInstanceHandle: Object, ): TextInstance { throw new Error('Not yet implemented.'); diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index b24bb68a2a8d1..62f993247cc96 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -41,10 +41,18 @@ type Instance = {| text: string | null, prop: any, hidden: boolean, + context: HostContext, |}; -type TextInstance = {|text: string, id: number, hidden: boolean|}; +type TextInstance = {| + text: string, + id: number, + hidden: boolean, + context: HostContext, +|}; +type HostContext = Object; const NO_CONTEXT = {}; +const UPPERCASE_CONTEXT = {}; const UPDATE_SIGNAL = {}; if (__DEV__) { Object.freeze(NO_CONTEXT); @@ -190,10 +198,11 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { type: type, children: keepChildren ? instance.children : [], text: shouldSetTextContent(type, newProps) - ? (newProps.children: any) + '' + ? computeText((newProps.children: any) + '', instance.context) : null, prop: newProps.prop, hidden: newProps.hidden === true, + context: instance.context, }; Object.defineProperty(clone, 'id', { value: clone.id, @@ -203,6 +212,10 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { value: clone.text, enumerable: false, }); + Object.defineProperty(clone, 'context', { + value: clone.context, + enumerable: false, + }); hostCloneCounter++; return clone; } @@ -216,12 +229,23 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { ); } + function computeText(rawText, hostContext) { + return hostContext === UPPERCASE_CONTEXT ? rawText.toUpperCase() : rawText; + } + const sharedHostConfig = { getRootHostContext() { return NO_CONTEXT; }, - getChildHostContext() { + getChildHostContext( + parentHostContext: HostContext, + type: string, + rootcontainerInstance: Container, + ) { + if (type === 'uppercase') { + return UPPERCASE_CONTEXT; + } return NO_CONTEXT; }, @@ -229,7 +253,12 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { return instance; }, - createInstance(type: string, props: Props): Instance { + createInstance( + type: string, + props: Props, + rootContainerInstance: Container, + hostContext: HostContext, + ): Instance { if (type === 'errorInCompletePhase') { throw new Error('Error in host config.'); } @@ -238,10 +267,11 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { type: type, children: [], text: shouldSetTextContent(type, props) - ? (props.children: any) + '' + ? computeText((props.children: any) + '', hostContext) : null, prop: props.prop, hidden: props.hidden === true, + context: hostContext, }; // Hide from unit tests Object.defineProperty(inst, 'id', {value: inst.id, enumerable: false}); @@ -249,6 +279,10 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { value: inst.text, enumerable: false, }); + Object.defineProperty(inst, 'context', { + value: inst.context, + enumerable: false, + }); return inst; }, @@ -298,9 +332,21 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { hostContext: Object, internalInstanceHandle: Object, ): TextInstance { - const inst = {text: text, id: instanceCounter++, hidden: false}; + if (hostContext === UPPERCASE_CONTEXT) { + text = text.toUpperCase(); + } + const inst = { + text: text, + id: instanceCounter++, + hidden: false, + context: hostContext, + }; // Hide from unit tests Object.defineProperty(inst, 'id', {value: inst.id, enumerable: false}); + Object.defineProperty(inst, 'context', { + value: inst.context, + enumerable: false, + }); return inst; }, @@ -343,7 +389,10 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { instance.prop = newProps.prop; instance.hidden = newProps.hidden === true; if (shouldSetTextContent(type, newProps)) { - instance.text = (newProps.children: any) + ''; + instance.text = computeText( + (newProps.children: any) + '', + instance.context, + ); } }, @@ -353,7 +402,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { newText: string, ): void { hostUpdateCounter++; - textInstance.text = newText; + textInstance.text = computeText(newText, textInstance.context); }, appendChild, @@ -453,23 +502,54 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { true, null, ); - clone.hidden = props.hidden; + clone.hidden = props.hidden === true; + return clone; + }, + + cloneHiddenTextInstance( + instance: TextInstance, + text: string, + internalInstanceHandle: Object, + ): TextInstance { + const clone = { + text: instance.text, + id: instanceCounter++, + hidden: true, + context: instance.context, + }; + // Hide from unit tests + Object.defineProperty(clone, 'id', { + value: clone.id, + enumerable: false, + }); + Object.defineProperty(clone, 'context', { + value: clone.context, + enumerable: false, + }); return clone; }, - createHiddenTextInstance( + cloneUnhiddenTextInstance( + instance: TextInstance, text: string, - rootContainerInstance: Container, - hostContext: Object, internalInstanceHandle: Object, ): TextInstance { - const inst = {text: text, id: instanceCounter++, hidden: true}; + const clone = { + text: instance.text, + id: instanceCounter++, + hidden: false, + context: instance.context, + }; // Hide from unit tests - Object.defineProperty(inst, 'id', { - value: inst.id, + Object.defineProperty(clone, 'id', { + value: clone.id, + enumerable: false, + }); + Object.defineProperty(clone, 'context', { + value: clone.context, enumerable: false, }); - return inst; + return clone; }, }; diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index 13e79828a92be..14345d3a30ad6 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -1131,6 +1131,13 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void { commitHookEffectList(UnmountMutation, MountMutation, finishedWork); return; } + case Profiler: { + return; + } + case SuspenseComponent: { + commitSuspenseComponent(finishedWork); + return; + } } commitContainer(finishedWork); @@ -1199,50 +1206,7 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void { return; } case SuspenseComponent: { - let newState: SuspenseState | null = finishedWork.memoizedState; - - let newDidTimeout; - let primaryChildParent = finishedWork; - if (newState === null) { - newDidTimeout = false; - } else { - newDidTimeout = true; - primaryChildParent = finishedWork.child; - if (newState.timedOutAt === NoWork) { - // If the children had not already timed out, record the time. - // This is used to compute the elapsed time during subsequent - // attempts to render the children. - newState.timedOutAt = requestCurrentTime(); - } - } - - if (primaryChildParent !== null) { - hideOrUnhideAllChildren(primaryChildParent, newDidTimeout); - } - - // If this boundary just timed out, then it will have a set of thenables. - // For each thenable, attach a listener so that when it resolves, React - // attempts to re-render the boundary in the primary (pre-timeout) state. - const thenables: Set | null = (finishedWork.updateQueue: any); - if (thenables !== null) { - finishedWork.updateQueue = null; - let retryCache = finishedWork.stateNode; - if (retryCache === null) { - retryCache = finishedWork.stateNode = new PossiblyWeakSet(); - } - thenables.forEach(thenable => { - // Memoize using the boundary fiber to prevent redundant listeners. - let retry = resolveRetryThenable.bind(null, finishedWork, thenable); - if (enableSchedulerTracing) { - retry = Schedule_tracing_wrap(retry); - } - if (!retryCache.has(thenable)) { - retryCache.add(thenable); - thenable.then(retry, retry); - } - }); - } - + commitSuspenseComponent(finishedWork); return; } case IncompleteClassComponent: { @@ -1258,6 +1222,52 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void { } } +function commitSuspenseComponent(finishedWork: Fiber) { + let newState: SuspenseState | null = finishedWork.memoizedState; + + let newDidTimeout; + let primaryChildParent = finishedWork; + if (newState === null) { + newDidTimeout = false; + } else { + newDidTimeout = true; + primaryChildParent = finishedWork.child; + if (newState.timedOutAt === NoWork) { + // If the children had not already timed out, record the time. + // This is used to compute the elapsed time during subsequent + // attempts to render the children. + newState.timedOutAt = requestCurrentTime(); + } + } + + if (supportsMutation && primaryChildParent !== null) { + hideOrUnhideAllChildren(primaryChildParent, newDidTimeout); + } + + // If this boundary just timed out, then it will have a set of thenables. + // For each thenable, attach a listener so that when it resolves, React + // attempts to re-render the boundary in the primary (pre-timeout) state. + const thenables: Set | null = (finishedWork.updateQueue: any); + if (thenables !== null) { + finishedWork.updateQueue = null; + let retryCache = finishedWork.stateNode; + if (retryCache === null) { + retryCache = finishedWork.stateNode = new PossiblyWeakSet(); + } + thenables.forEach(thenable => { + // Memoize using the boundary fiber to prevent redundant listeners. + let retry = resolveRetryThenable.bind(null, finishedWork, thenable); + if (enableSchedulerTracing) { + retry = Schedule_tracing_wrap(retry); + } + if (!retryCache.has(thenable)) { + retryCache.add(thenable); + thenable.then(retry, retry); + } + }); + } +} + function commitResetTextContent(current: Fiber) { if (!supportsMutation) { return; diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.js b/packages/react-reconciler/src/ReactFiberCompleteWork.js index 86d3ec7d86454..b1f374bb90bc6 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.js @@ -17,7 +17,6 @@ import type { Container, ChildSet, } from './ReactFiberHostConfig'; -import type {SuspenseState} from './ReactFiberSuspenseComponent'; import { IndeterminateComponent, @@ -53,7 +52,6 @@ import invariant from 'shared/invariant'; import { createInstance, createTextInstance, - createHiddenTextInstance, appendInitialChild, finalizeInitialChildren, prepareUpdate, @@ -62,6 +60,8 @@ import { cloneInstance, cloneHiddenInstance, cloneUnhiddenInstance, + cloneHiddenTextInstance, + cloneUnhiddenTextInstance, createContainerChildSet, appendChildToContainerChildSet, finalizeContainerChildren, @@ -228,22 +228,10 @@ if (supportsMutation) { let instance = node.stateNode; if (needsVisibilityToggle) { const text = node.memoizedProps; - const rootContainerInstance = getRootHostContainer(); - const currentHostContext = getHostContext(); if (isHidden) { - instance = createHiddenTextInstance( - text, - rootContainerInstance, - currentHostContext, - workInProgress, - ); + instance = cloneHiddenTextInstance(instance, text, node); } else { - instance = createTextInstance( - text, - rootContainerInstance, - currentHostContext, - workInProgress, - ); + instance = cloneUnhiddenTextInstance(instance, text, node); } node.stateNode = instance; } @@ -253,20 +241,19 @@ if (supportsMutation) { // down its children. Instead, we'll get insertions from each child in // the portal directly. } else if (node.tag === SuspenseComponent) { - const current = node.alternate; - if (current !== null) { - const oldState: SuspenseState = current.memoizedState; - const newState: SuspenseState = node.memoizedState; - const oldIsHidden = oldState !== null; - const newIsHidden = newState !== null; - if (oldIsHidden !== newIsHidden) { - // The placeholder either just timed out or switched back to the normal - // children after having previously timed out. Toggle the visibility of - // the direct host children. - const primaryChildParent = newIsHidden ? node.child : node; + if ((node.effectTag & Update) !== NoEffect) { + // Need to toggle the visibility of the primary children. + const newIsHidden = node.memoizedState !== null; + if (newIsHidden) { + const primaryChildParent = node.child; if (primaryChildParent !== null) { appendAllChildren(parent, primaryChildParent, true, newIsHidden); + node = primaryChildParent.sibling; + continue; } + } else { + const primaryChildParent = node; + appendAllChildren(parent, primaryChildParent, true, newIsHidden); // eslint-disable-next-line no-labels break branches; } @@ -331,22 +318,10 @@ if (supportsMutation) { let instance = node.stateNode; if (needsVisibilityToggle) { const text = node.memoizedProps; - const rootContainerInstance = getRootHostContainer(); - const currentHostContext = getHostContext(); if (isHidden) { - instance = createHiddenTextInstance( - text, - rootContainerInstance, - currentHostContext, - workInProgress, - ); + instance = cloneHiddenTextInstance(instance, text, node); } else { - instance = createTextInstance( - text, - rootContainerInstance, - currentHostContext, - workInProgress, - ); + instance = cloneUnhiddenTextInstance(instance, text, node); } node.stateNode = instance; } @@ -356,17 +331,11 @@ if (supportsMutation) { // down its children. Instead, we'll get insertions from each child in // the portal directly. } else if (node.tag === SuspenseComponent) { - const current = node.alternate; - if (current !== null) { - const oldState: SuspenseState = current.memoizedState; - const newState: SuspenseState = node.memoizedState; - const oldIsHidden = oldState !== null; - const newIsHidden = newState !== null; - if (oldIsHidden !== newIsHidden) { - // The placeholder either just timed out or switched back to the normal - // children after having previously timed out. Toggle the visibility of - // the direct host children. - const primaryChildParent = newIsHidden ? node.child : node; + if ((node.effectTag & Update) !== NoEffect) { + // Need to toggle the visibility of the primary children. + const newIsHidden = node.memoizedState !== null; + if (newIsHidden) { + const primaryChildParent = node.child; if (primaryChildParent !== null) { appendAllChildrenToContainer( containerChildSet, @@ -374,7 +343,17 @@ if (supportsMutation) { true, newIsHidden, ); + node = primaryChildParent.sibling; + continue; } + } else { + const primaryChildParent = node; + appendAllChildrenToContainer( + containerChildSet, + primaryChildParent, + true, + newIsHidden, + ); // eslint-disable-next-line no-labels break branches; } diff --git a/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js index fc49bd158aafc..637779adbd7d8 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js @@ -8,12 +8,9 @@ * @jest-environment node */ -// TODO: This does nothing since it was migrated from noop renderer to test -// renderer! Switch back to noop renderer, or add persistent mode to test -// renderer, or merge the two renderers into one somehow. -// runPlaceholderTests('ReactSuspensePlaceholder (mutation)', () => -// require('react-noop-renderer'), -// ); +runPlaceholderTests('ReactSuspensePlaceholder (mutation)', () => + require('react-noop-renderer'), +); runPlaceholderTests('ReactSuspensePlaceholder (persistence)', () => require('react-noop-renderer/persistent'), ); @@ -21,7 +18,7 @@ runPlaceholderTests('ReactSuspensePlaceholder (persistence)', () => function runPlaceholderTests(suiteLabel, loadReactNoop) { let Profiler; let React; - let ReactTestRenderer; + let ReactNoop; let Scheduler; let ReactFeatureFlags; let ReactCache; @@ -38,7 +35,7 @@ function runPlaceholderTests(suiteLabel, loadReactNoop) { ReactFeatureFlags.enableProfilerTimer = true; ReactFeatureFlags.replayFailedUnitOfWorkWithInvokeGuardedCallback = false; React = require('react'); - ReactTestRenderer = require('react-test-renderer'); + ReactNoop = loadReactNoop(); Scheduler = require('scheduler'); ReactCache = require('react-cache'); @@ -134,9 +131,7 @@ function runPlaceholderTests(suiteLabel, loadReactNoop) { } // Initial mount - const root = ReactTestRenderer.create(, { - unstable_isConcurrent: true, - }); + ReactNoop.render(); expect(Scheduler).toFlushAndYield([ 'A', @@ -144,14 +139,14 @@ function runPlaceholderTests(suiteLabel, loadReactNoop) { 'C', 'Loading...', ]); - expect(root).toMatchRenderedOutput(null); + expect(ReactNoop).toMatchRenderedOutput(null); jest.advanceTimersByTime(1000); expect(Scheduler).toHaveYielded(['Promise resolved [B]']); expect(Scheduler).toFlushAndYield(['A', 'B', 'C']); - expect(root).toMatchRenderedOutput( + expect(ReactNoop).toMatchRenderedOutput( B @@ -160,13 +155,20 @@ function runPlaceholderTests(suiteLabel, loadReactNoop) { ); // Update - root.update(); + ReactNoop.render(); expect(Scheduler).toFlushAndYield(['Suspend! [B2]', 'C', 'Loading...']); // Time out the update jest.advanceTimersByTime(750); expect(Scheduler).toFlushAndYield([]); - expect(root).toMatchRenderedOutput('Loading...'); + expect(ReactNoop).toMatchRenderedOutput( + + + + + Loading... + , + ); // Resolve the promise jest.advanceTimersByTime(1000); @@ -175,7 +177,7 @@ function runPlaceholderTests(suiteLabel, loadReactNoop) { // Render the final update. A should still be hidden, because it was // given a `hidden` prop. - expect(root).toMatchRenderedOutput( + expect(ReactNoop).toMatchRenderedOutput( B2 @@ -196,9 +198,7 @@ function runPlaceholderTests(suiteLabel, loadReactNoop) { } // Initial mount - const root = ReactTestRenderer.create(, { - unstable_isConcurrent: true, - }); + ReactNoop.render(); expect(Scheduler).toFlushAndYield([ 'A', @@ -207,15 +207,15 @@ function runPlaceholderTests(suiteLabel, loadReactNoop) { 'Loading...', ]); - expect(root).toMatchRenderedOutput(null); + expect(ReactNoop).toMatchRenderedOutput(null); jest.advanceTimersByTime(1000); expect(Scheduler).toHaveYielded(['Promise resolved [B]']); expect(Scheduler).toFlushAndYield(['A', 'B', 'C']); - expect(root).toMatchRenderedOutput('ABC'); + expect(ReactNoop).toMatchRenderedOutput('ABC'); // Update - root.update(); + ReactNoop.render(); expect(Scheduler).toFlushAndYield([ 'A', 'Suspend! [B2]', @@ -225,7 +225,7 @@ function runPlaceholderTests(suiteLabel, loadReactNoop) { // Time out the update jest.advanceTimersByTime(750); expect(Scheduler).toFlushAndYield([]); - expect(root).toMatchRenderedOutput('Loading...'); + expect(ReactNoop).toMatchRenderedOutput('Loading...'); // Resolve the promise jest.advanceTimersByTime(1000); @@ -234,7 +234,64 @@ function runPlaceholderTests(suiteLabel, loadReactNoop) { // Render the final update. A should still be hidden, because it was // given a `hidden` prop. - expect(root).toMatchRenderedOutput('AB2C'); + expect(ReactNoop).toMatchRenderedOutput('AB2C'); + }); + + it('preserves host context for text nodes', () => { + function App(props) { + return ( + // uppercase is a special type that causes React Noop to render child + // text nodes as uppercase. + + }> + + + + + + ); + } + + // Initial mount + ReactNoop.render(); + + expect(Scheduler).toFlushAndYield([ + 'a', + 'Suspend! [b]', + 'c', + 'Loading...', + ]); + + expect(ReactNoop).toMatchRenderedOutput(null); + + jest.advanceTimersByTime(1000); + expect(Scheduler).toHaveYielded(['Promise resolved [b]']); + expect(Scheduler).toFlushAndYield(['a', 'b', 'c']); + expect(ReactNoop).toMatchRenderedOutput(ABC); + + // Update + ReactNoop.render(); + expect(Scheduler).toFlushAndYield([ + 'a', + 'Suspend! [b2]', + 'c', + 'Loading...', + ]); + // Time out the update + jest.advanceTimersByTime(750); + expect(Scheduler).toFlushAndYield([]); + expect(ReactNoop).toMatchRenderedOutput( + LOADING..., + ); + + // Resolve the promise + jest.advanceTimersByTime(1000); + expect(Scheduler).toHaveYielded(['Promise resolved [b2]']); + expect(Scheduler).toFlushAndYield(['a', 'b2', 'c']); + + // Render the final update. A should still be hidden, because it was + // given a `hidden` prop. + expect(ReactNoop).toMatchRenderedOutput(AB2C); }); describe('profiler durations', () => { @@ -272,8 +329,15 @@ function runPlaceholderTests(suiteLabel, loadReactNoop) { describe('when suspending during mount', () => { it('properly accounts for base durations when a suspended times out in a sync tree', () => { - const root = ReactTestRenderer.create(); - expect(root.toJSON()).toEqual('Loading...'); + ReactNoop.renderLegacySyncRoot(); + expect(Scheduler).toHaveYielded([ + 'App', + 'Suspending', + 'Suspend! [Loaded]', + 'Text', + 'Fallback', + ]); + expect(ReactNoop).toMatchRenderedOutput('Loading...'); expect(onRender).toHaveBeenCalledTimes(1); // Initial mount only shows the "Loading..." Fallback. @@ -284,7 +348,11 @@ function runPlaceholderTests(suiteLabel, loadReactNoop) { jest.advanceTimersByTime(1000); - expect(root.toJSON()).toEqual(['Loaded', 'Text']); + expect(Scheduler).toHaveYielded([ + 'Promise resolved [Loaded]', + 'Loaded', + ]); + expect(ReactNoop).toMatchRenderedOutput('LoadedText'); expect(onRender).toHaveBeenCalledTimes(2); // When the suspending data is resolved and our final UI is rendered, @@ -295,9 +363,7 @@ function runPlaceholderTests(suiteLabel, loadReactNoop) { }); it('properly accounts for base durations when a suspended times out in a concurrent tree', () => { - const root = ReactTestRenderer.create(, { - unstable_isConcurrent: true, - }); + ReactNoop.render(); expect(Scheduler).toFlushAndYield([ 'App', @@ -306,11 +372,11 @@ function runPlaceholderTests(suiteLabel, loadReactNoop) { 'Text', 'Fallback', ]); - expect(root).toMatchRenderedOutput(null); + expect(ReactNoop).toMatchRenderedOutput(null); // Show the fallback UI. jest.advanceTimersByTime(750); - expect(root).toMatchRenderedOutput('Loading...'); + expect(ReactNoop).toMatchRenderedOutput('Loading...'); expect(onRender).toHaveBeenCalledTimes(1); // Initial mount only shows the "Loading..." Fallback. @@ -323,7 +389,7 @@ function runPlaceholderTests(suiteLabel, loadReactNoop) { jest.advanceTimersByTime(250); expect(Scheduler).toHaveYielded(['Promise resolved [Loaded]']); expect(Scheduler).toFlushAndYield(['Suspending', 'Loaded', 'Text']); - expect(root).toMatchRenderedOutput('LoadedText'); + expect(ReactNoop).toMatchRenderedOutput('LoadedText'); expect(onRender).toHaveBeenCalledTimes(2); // When the suspending data is resolved and our final UI is rendered, @@ -335,10 +401,11 @@ function runPlaceholderTests(suiteLabel, loadReactNoop) { describe('when suspending during update', () => { it('properly accounts for base durations when a suspended times out in a sync tree', () => { - const root = ReactTestRenderer.create( + ReactNoop.renderLegacySyncRoot( , ); - expect(root.toJSON()).toEqual('Text'); + expect(Scheduler).toHaveYielded(['App', 'Text']); + expect(ReactNoop).toMatchRenderedOutput('Text'); expect(onRender).toHaveBeenCalledTimes(1); // Initial mount only shows the "Text" text. @@ -346,8 +413,15 @@ function runPlaceholderTests(suiteLabel, loadReactNoop) { expect(onRender.mock.calls[0][2]).toBe(5); expect(onRender.mock.calls[0][3]).toBe(5); - root.update(); - expect(root.toJSON()).toEqual('Loading...'); + ReactNoop.render(); + expect(Scheduler).toHaveYielded([ + 'App', + 'Suspending', + 'Suspend! [Loaded]', + 'Text', + 'Fallback', + ]); + expect(ReactNoop).toMatchRenderedOutput('Loading...'); expect(onRender).toHaveBeenCalledTimes(2); // The suspense update should only show the "Loading..." Fallback. @@ -356,10 +430,17 @@ function runPlaceholderTests(suiteLabel, loadReactNoop) { expect(onRender.mock.calls[1][2]).toBe(18); expect(onRender.mock.calls[1][3]).toBe(18); - root.update( + ReactNoop.renderLegacySyncRoot( , ); - expect(root.toJSON()).toEqual('Loading...'); + expect(Scheduler).toHaveYielded([ + 'App', + 'Suspending', + 'Suspend! [Loaded]', + 'New', + 'Fallback', + ]); + expect(ReactNoop).toMatchRenderedOutput('Loading...'); expect(onRender).toHaveBeenCalledTimes(3); // If we force another update while still timed out, @@ -370,7 +451,11 @@ function runPlaceholderTests(suiteLabel, loadReactNoop) { jest.advanceTimersByTime(1000); - expect(root.toJSON()).toEqual(['Loaded', 'New']); + expect(Scheduler).toHaveYielded([ + 'Promise resolved [Loaded]', + 'Loaded', + ]); + expect(ReactNoop).toMatchRenderedOutput('LoadedNew'); expect(onRender).toHaveBeenCalledTimes(4); // When the suspending data is resolved and our final UI is rendered, @@ -381,15 +466,12 @@ function runPlaceholderTests(suiteLabel, loadReactNoop) { }); it('properly accounts for base durations when a suspended times out in a concurrent tree', () => { - const root = ReactTestRenderer.create( + ReactNoop.render( , - { - unstable_isConcurrent: true, - }, ); expect(Scheduler).toFlushAndYield(['App', 'Text']); - expect(root).toMatchRenderedOutput('Text'); + expect(ReactNoop).toMatchRenderedOutput('Text'); expect(onRender).toHaveBeenCalledTimes(1); // Initial mount only shows the "Text" text. @@ -397,7 +479,7 @@ function runPlaceholderTests(suiteLabel, loadReactNoop) { expect(onRender.mock.calls[0][2]).toBe(5); expect(onRender.mock.calls[0][3]).toBe(5); - root.update(); + ReactNoop.render(); expect(Scheduler).toFlushAndYield([ 'App', 'Suspending', @@ -405,11 +487,11 @@ function runPlaceholderTests(suiteLabel, loadReactNoop) { 'Text', 'Fallback', ]); - expect(root).toMatchRenderedOutput('Text'); + expect(ReactNoop).toMatchRenderedOutput('Text'); // Show the fallback UI. jest.advanceTimersByTime(750); - expect(root).toMatchRenderedOutput('Loading...'); + expect(ReactNoop).toMatchRenderedOutput('Loading...'); expect(onRender).toHaveBeenCalledTimes(2); // The suspense update should only show the "Loading..." Fallback. @@ -421,7 +503,7 @@ function runPlaceholderTests(suiteLabel, loadReactNoop) { expect(onRender.mock.calls[1][3]).toBe(15); // Update again while timed out. - root.update( + ReactNoop.render( , ); expect(Scheduler).toFlushAndYield([ @@ -431,7 +513,7 @@ function runPlaceholderTests(suiteLabel, loadReactNoop) { 'New', 'Fallback', ]); - expect(root).toMatchRenderedOutput('Loading...'); + expect(ReactNoop).toMatchRenderedOutput('Loading...'); expect(onRender).toHaveBeenCalledTimes(2); // Resolve the pending promise. diff --git a/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js b/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js index 994ca852ad327..4a68193c8534d 100644 --- a/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js +++ b/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js @@ -92,7 +92,9 @@ export const finalizeContainerChildren = export const replaceContainerChildren = $$$hostConfig.replaceContainerChildren; export const cloneHiddenInstance = $$$hostConfig.cloneHiddenInstance; export const cloneUnhiddenInstance = $$$hostConfig.cloneUnhiddenInstance; -export const createHiddenTextInstance = $$$hostConfig.createHiddenTextInstance; +export const cloneHiddenTextInstance = $$$hostConfig.cloneHiddenTextInstance; +export const cloneUnhiddenTextInstance = + $$$hostConfig.cloneUnhiddenTextInstance; // ------------------- // Hydration diff --git a/packages/shared/HostConfigWithNoPersistence.js b/packages/shared/HostConfigWithNoPersistence.js index 397df41ef0a7b..ffd8342708e34 100644 --- a/packages/shared/HostConfigWithNoPersistence.js +++ b/packages/shared/HostConfigWithNoPersistence.js @@ -30,4 +30,5 @@ export const finalizeContainerChildren = shim; export const replaceContainerChildren = shim; export const cloneHiddenInstance = shim; export const cloneUnhiddenInstance = shim; -export const createHiddenTextInstance = shim; +export const cloneHiddenTextInstance = shim; +export const cloneUnhiddenTextInstance = shim; From bc8bd24c145d386be7f014f04fc927067d18412a Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 11 Mar 2019 10:56:34 -0700 Subject: [PATCH 13/22] Run persistent mode tests in CI (#15029) * Add command to run tests in persistent mode * Convert Suspense fuzz tester to use noop renderer So we can run it in persistent mode, too. * Don't mutate stateNode in appendAllChildren We can't mutate the stateNode in appendAllChildren because the children could be current. This is a bit weird because now the child that we append is different from the one on the fiber stateNode. I think this makes conceptual sense, but I suspect this likely breaks an assumption in Fabric. With this approach, we no longer need to clone to unhide the children, so I removed those host config methods. Fixes bug surfaced by fuzz tester. (The test case that failed was the one that's already hard coded.) * In persistent mode, disable test that reads a ref Refs behave differently in persistent mode. I added a TODO to write a persistent mode version of this test. * Run persistent mode tests in CI * test-persistent should skip files without noop If a file doesn't reference react-noop-renderer, we shouldn't bother running it in persistent mode, since the results will be identical to the normal test run. * Remove module constructor from placeholder tests We don't need this now that we have the ability to run any test file in either mutation or persistent mode. * Revert "test-persistent should skip files without noop" Seb objected to adding shelljs as a dep and I'm too lazy to worry about Windows support so whatever I'll just revert this. * Delete duplicate file --- package.json | 1 + .../src/ReactFabricHostConfig.js | 27 - .../src/createReactNoop.js | 43 - .../src/ReactFiberCompleteWork.js | 128 ++- .../ReactSuspenseFuzz-test.internal.js | 53 +- .../ReactSuspensePlaceholder-test.internal.js | 964 +++++++++--------- ...tSuspenseWithNoopRenderer-test.internal.js | 63 +- .../src/forks/ReactFiberHostConfig.custom.js | 3 - .../shared/HostConfigWithNoPersistence.js | 2 - scripts/circleci/test_entry_point.sh | 1 + scripts/jest/config.source-persistent.js | 18 + scripts/jest/setupTests.persistent.js | 7 + 12 files changed, 611 insertions(+), 699 deletions(-) create mode 100644 scripts/jest/config.source-persistent.js create mode 100644 scripts/jest/setupTests.persistent.js diff --git a/package.json b/package.json index bf399df5078d6..29e3aaea389dc 100644 --- a/package.json +++ b/package.json @@ -100,6 +100,7 @@ "postinstall": "node node_modules/fbjs-scripts/node/check-dev-engines.js package.json && node ./scripts/flow/createFlowConfigs.js", "debug-test": "cross-env NODE_ENV=development node --inspect-brk node_modules/.bin/jest --config ./scripts/jest/config.source.js --runInBand", "test": "cross-env NODE_ENV=development jest --config ./scripts/jest/config.source.js", + "test-persistent": "cross-env NODE_ENV=development jest --config ./scripts/jest/config.source-persistent.js", "test-fire": "cross-env NODE_ENV=development jest --config ./scripts/jest/config.source-fire.js", "test-prod": "cross-env NODE_ENV=production jest --config ./scripts/jest/config.source.js", "test-fire-prod": "cross-env NODE_ENV=production jest --config ./scripts/jest/config.source-fire.js", diff --git a/packages/react-native-renderer/src/ReactFabricHostConfig.js b/packages/react-native-renderer/src/ReactFabricHostConfig.js index 0eed32d066246..c1268eb663edb 100644 --- a/packages/react-native-renderer/src/ReactFabricHostConfig.js +++ b/packages/react-native-renderer/src/ReactFabricHostConfig.js @@ -387,25 +387,6 @@ export function cloneHiddenInstance( }; } -export function cloneUnhiddenInstance( - instance: Instance, - type: string, - props: Props, - internalInstanceHandle: Object, -): Instance { - const viewConfig = instance.canonical.viewConfig; - const node = instance.node; - const updatePayload = diff( - {...props, style: [props.style, {display: 'none'}]}, - props, - viewConfig.validAttributes, - ); - return { - node: cloneNodeWithNewProps(node, updatePayload), - canonical: instance.canonical, - }; -} - export function cloneHiddenTextInstance( instance: Instance, text: string, @@ -414,14 +395,6 @@ export function cloneHiddenTextInstance( throw new Error('Not yet implemented.'); } -export function cloneUnhiddenTextInstance( - instance: Instance, - text: string, - internalInstanceHandle: Object, -): TextInstance { - throw new Error('Not yet implemented.'); -} - export function createContainerChildSet(container: Container): ChildSet { return createChildNodeSet(container); } diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index 62f993247cc96..f3872ac483b57 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -486,26 +486,6 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { return clone; }, - cloneUnhiddenInstance( - instance: Instance, - type: string, - props: Props, - internalInstanceHandle: Object, - ): Instance { - const clone = cloneInstance( - instance, - null, - type, - props, - props, - internalInstanceHandle, - true, - null, - ); - clone.hidden = props.hidden === true; - return clone; - }, - cloneHiddenTextInstance( instance: TextInstance, text: string, @@ -528,29 +508,6 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { }); return clone; }, - - cloneUnhiddenTextInstance( - instance: TextInstance, - text: string, - internalInstanceHandle: Object, - ): TextInstance { - const clone = { - text: instance.text, - id: instanceCounter++, - hidden: false, - context: instance.context, - }; - // Hide from unit tests - Object.defineProperty(clone, 'id', { - value: clone.id, - enumerable: false, - }); - Object.defineProperty(clone, 'context', { - value: clone.context, - enumerable: false, - }); - return clone; - }, }; const NoopRenderer = reconciler(hostConfig); diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.js b/packages/react-reconciler/src/ReactFiberCompleteWork.js index b1f374bb90bc6..03ef368fc3a88 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.js @@ -59,9 +59,7 @@ import { supportsPersistence, cloneInstance, cloneHiddenInstance, - cloneUnhiddenInstance, cloneHiddenTextInstance, - cloneUnhiddenTextInstance, createContainerChildSet, appendChildToContainerChildSet, finalizeContainerChildren, @@ -209,31 +207,19 @@ if (supportsMutation) { // eslint-disable-next-line no-labels branches: if (node.tag === HostComponent) { let instance = node.stateNode; - if (needsVisibilityToggle) { + if (needsVisibilityToggle && isHidden) { + // This child is inside a timed out tree. Hide it. const props = node.memoizedProps; const type = node.type; - if (isHidden) { - // This child is inside a timed out tree. Hide it. - instance = cloneHiddenInstance(instance, type, props, node); - } else { - // This child was previously inside a timed out tree. If it was not - // updated during this render, it may need to be unhidden. Clone - // again to be sure. - instance = cloneUnhiddenInstance(instance, type, props, node); - } - node.stateNode = instance; + instance = cloneHiddenInstance(instance, type, props, node); } appendInitialChild(parent, instance); } else if (node.tag === HostText) { let instance = node.stateNode; - if (needsVisibilityToggle) { + if (needsVisibilityToggle && isHidden) { + // This child is inside a timed out tree. Hide it. const text = node.memoizedProps; - if (isHidden) { - instance = cloneHiddenTextInstance(instance, text, node); - } else { - instance = cloneUnhiddenTextInstance(instance, text, node); - } - node.stateNode = instance; + instance = cloneHiddenTextInstance(instance, text, node); } appendInitialChild(parent, instance); } else if (node.tag === HostPortal) { @@ -247,15 +233,22 @@ if (supportsMutation) { if (newIsHidden) { const primaryChildParent = node.child; if (primaryChildParent !== null) { - appendAllChildren(parent, primaryChildParent, true, newIsHidden); - node = primaryChildParent.sibling; - continue; + if (primaryChildParent.child !== null) { + primaryChildParent.child.return = primaryChildParent; + appendAllChildren( + parent, + primaryChildParent, + true, + newIsHidden, + ); + } + const fallbackChildParent = primaryChildParent.sibling; + if (fallbackChildParent !== null) { + fallbackChildParent.return = node; + node = fallbackChildParent; + continue; + } } - } else { - const primaryChildParent = node; - appendAllChildren(parent, primaryChildParent, true, newIsHidden); - // eslint-disable-next-line no-labels - break branches; } } if (node.child !== null) { @@ -299,31 +292,19 @@ if (supportsMutation) { // eslint-disable-next-line no-labels branches: if (node.tag === HostComponent) { let instance = node.stateNode; - if (needsVisibilityToggle) { + if (needsVisibilityToggle && isHidden) { + // This child is inside a timed out tree. Hide it. const props = node.memoizedProps; const type = node.type; - if (isHidden) { - // This child is inside a timed out tree. Hide it. - instance = cloneHiddenInstance(instance, type, props, node); - } else { - // This child was previously inside a timed out tree. If it was not - // updated during this render, it may need to be unhidden. Clone - // again to be sure. - instance = cloneUnhiddenInstance(instance, type, props, node); - } - node.stateNode = instance; + instance = cloneHiddenInstance(instance, type, props, node); } appendChildToContainerChildSet(containerChildSet, instance); } else if (node.tag === HostText) { let instance = node.stateNode; - if (needsVisibilityToggle) { + if (needsVisibilityToggle && isHidden) { + // This child is inside a timed out tree. Hide it. const text = node.memoizedProps; - if (isHidden) { - instance = cloneHiddenTextInstance(instance, text, node); - } else { - instance = cloneUnhiddenTextInstance(instance, text, node); - } - node.stateNode = instance; + instance = cloneHiddenTextInstance(instance, text, node); } appendChildToContainerChildSet(containerChildSet, instance); } else if (node.tag === HostPortal) { @@ -337,25 +318,22 @@ if (supportsMutation) { if (newIsHidden) { const primaryChildParent = node.child; if (primaryChildParent !== null) { - appendAllChildrenToContainer( - containerChildSet, - primaryChildParent, - true, - newIsHidden, - ); - node = primaryChildParent.sibling; - continue; + if (primaryChildParent.child !== null) { + primaryChildParent.child.return = primaryChildParent; + appendAllChildrenToContainer( + containerChildSet, + primaryChildParent, + true, + newIsHidden, + ); + } + const fallbackChildParent = primaryChildParent.sibling; + if (fallbackChildParent !== null) { + fallbackChildParent.return = node; + node = fallbackChildParent; + continue; + } } - } else { - const primaryChildParent = node; - appendAllChildrenToContainer( - containerChildSet, - primaryChildParent, - true, - newIsHidden, - ); - // eslint-disable-next-line no-labels - break branches; } } if (node.child !== null) { @@ -714,11 +692,23 @@ function completeWork( } } - if (nextDidTimeout || prevDidTimeout) { - // If the children are hidden, or if they were previous hidden, schedule - // an effect to toggle their visibility. This is also used to attach a - // retry listener to the promise. - workInProgress.effectTag |= Update; + if (supportsPersistence) { + if (nextDidTimeout) { + // If this boundary just timed out, schedule an effect to attach a + // retry listener to the proimse. This flag is also used to hide the + // primary children. + workInProgress.effectTag |= Update; + } + } + if (supportsMutation) { + if (nextDidTimeout || prevDidTimeout) { + // If this boundary just timed out, schedule an effect to attach a + // retry listener to the proimse. This flag is also used to hide the + // primary children. In mutation mode, we also need the flag to + // *unhide* children that were previously hidden, so check if the + // is currently timed out, too. + workInProgress.effectTag |= Update; + } } break; } diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseFuzz-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseFuzz-test.internal.js index 079f70f3dbf5f..22c2c051af38d 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseFuzz-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseFuzz-test.internal.js @@ -1,6 +1,6 @@ let React; let Suspense; -let ReactTestRenderer; +let ReactNoop; let Scheduler; let ReactFeatureFlags; let Random; @@ -26,7 +26,7 @@ describe('ReactSuspenseFuzz', () => { ReactFeatureFlags.replayFailedUnitOfWorkWithInvokeGuardedCallback = false; React = require('react'); Suspense = React.Suspense; - ReactTestRenderer = require('react-test-renderer'); + ReactNoop = require('react-noop-renderer'); Scheduler = require('scheduler'); Random = require('random-seed'); }); @@ -143,28 +143,16 @@ describe('ReactSuspenseFuzz', () => { return resolvedText; } - function renderToRoot( - root, - children, - {shouldSuspend} = {shouldSuspend: true}, - ) { - root.update( - - {children} - , - ); + function resolveAllTasks() { Scheduler.unstable_flushWithoutYielding(); - let elapsedTime = 0; while (pendingTasks && pendingTasks.size > 0) { if ((elapsedTime += 1000) > 1000000) { throw new Error('Something did not resolve properly.'); } - ReactTestRenderer.act(() => jest.advanceTimersByTime(1000)); + ReactNoop.act(() => jest.advanceTimersByTime(1000)); Scheduler.unstable_flushWithoutYielding(); } - - return root.toJSON(); } function testResolvedOutput(unwrappedChildren) { @@ -172,25 +160,32 @@ describe('ReactSuspenseFuzz', () => { {unwrappedChildren} ); - const expectedRoot = ReactTestRenderer.create(null); - const expectedOutput = renderToRoot(expectedRoot, children, { - shouldSuspend: false, - }); - expectedRoot.unmount(); + resetCache(); + ReactNoop.renderToRootWithID( + + {children} + , + 'expected', + ); + resolveAllTasks(); + const expectedOutput = ReactNoop.getChildrenAsJSX('expected'); + ReactNoop.renderToRootWithID(null, 'expected'); + Scheduler.unstable_flushWithoutYielding(); resetCache(); - const syncRoot = ReactTestRenderer.create(null); - const syncOutput = renderToRoot(syncRoot, children); + ReactNoop.renderLegacySyncRoot(children); + resolveAllTasks(); + const syncOutput = ReactNoop.getChildrenAsJSX(); expect(syncOutput).toEqual(expectedOutput); - syncRoot.unmount(); + ReactNoop.renderLegacySyncRoot(null); resetCache(); - const concurrentRoot = ReactTestRenderer.create(null, { - unstable_isConcurrent: true, - }); - const concurrentOutput = renderToRoot(concurrentRoot, children); + ReactNoop.renderToRootWithID(children, 'concurrent'); + Scheduler.unstable_flushWithoutYielding(); + resolveAllTasks(); + const concurrentOutput = ReactNoop.getChildrenAsJSX('concurrent'); expect(concurrentOutput).toEqual(expectedOutput); - concurrentRoot.unmount(); + ReactNoop.renderToRootWithID(null, 'concurrent'); Scheduler.unstable_flushWithoutYielding(); } diff --git a/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js index 637779adbd7d8..fdd2b717aae37 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js @@ -8,532 +8,504 @@ * @jest-environment node */ -runPlaceholderTests('ReactSuspensePlaceholder (mutation)', () => - require('react-noop-renderer'), -); -runPlaceholderTests('ReactSuspensePlaceholder (persistence)', () => - require('react-noop-renderer/persistent'), -); - -function runPlaceholderTests(suiteLabel, loadReactNoop) { - let Profiler; - let React; - let ReactNoop; - let Scheduler; - let ReactFeatureFlags; - let ReactCache; - let Suspense; - let TextResource; - let textResourceShouldFail; - - describe(suiteLabel, () => { - beforeEach(() => { - jest.resetModules(); - - ReactFeatureFlags = require('shared/ReactFeatureFlags'); - ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; - ReactFeatureFlags.enableProfilerTimer = true; - ReactFeatureFlags.replayFailedUnitOfWorkWithInvokeGuardedCallback = false; - React = require('react'); - ReactNoop = loadReactNoop(); - Scheduler = require('scheduler'); - ReactCache = require('react-cache'); - - Profiler = React.unstable_Profiler; - Suspense = React.Suspense; - - TextResource = ReactCache.unstable_createResource(([text, ms = 0]) => { - let listeners = null; - let status = 'pending'; - let value = null; - return { - then(resolve, reject) { - switch (status) { - case 'pending': { - if (listeners === null) { - listeners = [{resolve, reject}]; - setTimeout(() => { - if (textResourceShouldFail) { - Scheduler.yieldValue(`Promise rejected [${text}]`); - status = 'rejected'; - value = new Error('Failed to load: ' + text); - listeners.forEach(listener => listener.reject(value)); - } else { - Scheduler.yieldValue(`Promise resolved [${text}]`); - status = 'resolved'; - value = text; - listeners.forEach(listener => listener.resolve(value)); - } - }, ms); - } else { - listeners.push({resolve, reject}); - } - break; - } - case 'resolved': { - resolve(value); - break; - } - case 'rejected': { - reject(value); - break; +let Profiler; +let React; +let ReactNoop; +let Scheduler; +let ReactFeatureFlags; +let ReactCache; +let Suspense; +let TextResource; +let textResourceShouldFail; + +describe('ReactSuspensePlaceholder', () => { + beforeEach(() => { + jest.resetModules(); + + ReactFeatureFlags = require('shared/ReactFeatureFlags'); + ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; + ReactFeatureFlags.enableProfilerTimer = true; + ReactFeatureFlags.replayFailedUnitOfWorkWithInvokeGuardedCallback = false; + React = require('react'); + ReactNoop = require('react-noop-renderer'); + Scheduler = require('scheduler'); + ReactCache = require('react-cache'); + + Profiler = React.unstable_Profiler; + Suspense = React.Suspense; + + TextResource = ReactCache.unstable_createResource(([text, ms = 0]) => { + let listeners = null; + let status = 'pending'; + let value = null; + return { + then(resolve, reject) { + switch (status) { + case 'pending': { + if (listeners === null) { + listeners = [{resolve, reject}]; + setTimeout(() => { + if (textResourceShouldFail) { + Scheduler.yieldValue(`Promise rejected [${text}]`); + status = 'rejected'; + value = new Error('Failed to load: ' + text); + listeners.forEach(listener => listener.reject(value)); + } else { + Scheduler.yieldValue(`Promise resolved [${text}]`); + status = 'resolved'; + value = text; + listeners.forEach(listener => listener.resolve(value)); + } + }, ms); + } else { + listeners.push({resolve, reject}); } + break; } - }, - }; - }, ([text, ms]) => text); - textResourceShouldFail = false; - }); + case 'resolved': { + resolve(value); + break; + } + case 'rejected': { + reject(value); + break; + } + } + }, + }; + }, ([text, ms]) => text); + textResourceShouldFail = false; + }); - function Text({fakeRenderDuration = 0, text = 'Text'}) { - Scheduler.advanceTime(fakeRenderDuration); + function Text({fakeRenderDuration = 0, text = 'Text'}) { + Scheduler.advanceTime(fakeRenderDuration); + Scheduler.yieldValue(text); + return text; + } + + function AsyncText({fakeRenderDuration = 0, ms, text}) { + Scheduler.advanceTime(fakeRenderDuration); + try { + TextResource.read([text, ms]); Scheduler.yieldValue(text); return text; + } catch (promise) { + if (typeof promise.then === 'function') { + Scheduler.yieldValue(`Suspend! [${text}]`); + } else { + Scheduler.yieldValue(`Error! [${text}]`); + } + throw promise; } + } - function AsyncText({fakeRenderDuration = 0, ms, text}) { - Scheduler.advanceTime(fakeRenderDuration); - try { - TextResource.read([text, ms]); + it('times out children that are already hidden', () => { + class HiddenText extends React.PureComponent { + render() { + const text = this.props.text; Scheduler.yieldValue(text); - return text; - } catch (promise) { - if (typeof promise.then === 'function') { - Scheduler.yieldValue(`Suspend! [${text}]`); - } else { - Scheduler.yieldValue(`Error! [${text}]`); - } - throw promise; + return ; } } - it('times out children that are already hidden', () => { - class HiddenText extends React.PureComponent { - render() { - const text = this.props.text; - Scheduler.yieldValue(text); - return ; - } - } - - function App(props) { - return ( - }> - - - - - - - - - ); - } - - // Initial mount - ReactNoop.render(); - - expect(Scheduler).toFlushAndYield([ - 'A', - 'Suspend! [B]', - 'C', - 'Loading...', - ]); - expect(ReactNoop).toMatchRenderedOutput(null); - - jest.advanceTimersByTime(1000); - expect(Scheduler).toHaveYielded(['Promise resolved [B]']); - - expect(Scheduler).toFlushAndYield(['A', 'B', 'C']); - - expect(ReactNoop).toMatchRenderedOutput( - - - B - C - , + function App(props) { + return ( + }> + + + + + + + + ); + } - // Update - ReactNoop.render(); - expect(Scheduler).toFlushAndYield(['Suspend! [B2]', 'C', 'Loading...']); - - // Time out the update - jest.advanceTimersByTime(750); - expect(Scheduler).toFlushAndYield([]); - expect(ReactNoop).toMatchRenderedOutput( - - - - - Loading... - , - ); + // Initial mount + ReactNoop.render(); + + expect(Scheduler).toFlushAndYield(['A', 'Suspend! [B]', 'C', 'Loading...']); + expect(ReactNoop).toMatchRenderedOutput(null); + + jest.advanceTimersByTime(1000); + expect(Scheduler).toHaveYielded(['Promise resolved [B]']); + + expect(Scheduler).toFlushAndYield(['A', 'B', 'C']); + + expect(ReactNoop).toMatchRenderedOutput( + + + B + C + , + ); + + // Update + ReactNoop.render(); + expect(Scheduler).toFlushAndYield(['Suspend! [B2]', 'C', 'Loading...']); + + // Time out the update + jest.advanceTimersByTime(750); + expect(Scheduler).toFlushAndYield([]); + expect(ReactNoop).toMatchRenderedOutput( + + + + + Loading... + , + ); + + // Resolve the promise + jest.advanceTimersByTime(1000); + expect(Scheduler).toHaveYielded(['Promise resolved [B2]']); + expect(Scheduler).toFlushAndYield(['B2', 'C']); + + // Render the final update. A should still be hidden, because it was + // given a `hidden` prop. + expect(ReactNoop).toMatchRenderedOutput( + + + B2 + C + , + ); + }); - // Resolve the promise - jest.advanceTimersByTime(1000); - expect(Scheduler).toHaveYielded(['Promise resolved [B2]']); - expect(Scheduler).toFlushAndYield(['B2', 'C']); - - // Render the final update. A should still be hidden, because it was - // given a `hidden` prop. - expect(ReactNoop).toMatchRenderedOutput( - - - B2 - C - , + it('times out text nodes', async () => { + function App(props) { + return ( + }> + + + + ); - }); + } - it('times out text nodes', async () => { - function App(props) { - return ( + // Initial mount + ReactNoop.render(); + + expect(Scheduler).toFlushAndYield(['A', 'Suspend! [B]', 'C', 'Loading...']); + + expect(ReactNoop).toMatchRenderedOutput(null); + + jest.advanceTimersByTime(1000); + expect(Scheduler).toHaveYielded(['Promise resolved [B]']); + expect(Scheduler).toFlushAndYield(['A', 'B', 'C']); + expect(ReactNoop).toMatchRenderedOutput('ABC'); + + // Update + ReactNoop.render(); + expect(Scheduler).toFlushAndYield([ + 'A', + 'Suspend! [B2]', + 'C', + 'Loading...', + ]); + // Time out the update + jest.advanceTimersByTime(750); + expect(Scheduler).toFlushAndYield([]); + expect(ReactNoop).toMatchRenderedOutput('Loading...'); + + // Resolve the promise + jest.advanceTimersByTime(1000); + expect(Scheduler).toHaveYielded(['Promise resolved [B2]']); + expect(Scheduler).toFlushAndYield(['A', 'B2', 'C']); + + // Render the final update. A should still be hidden, because it was + // given a `hidden` prop. + expect(ReactNoop).toMatchRenderedOutput('AB2C'); + }); + + it('preserves host context for text nodes', () => { + function App(props) { + return ( + // uppercase is a special type that causes React Noop to render child + // text nodes as uppercase. + }> - + - + - ); - } + + ); + } - // Initial mount - ReactNoop.render(); - - expect(Scheduler).toFlushAndYield([ - 'A', - 'Suspend! [B]', - 'C', - 'Loading...', - ]); - - expect(ReactNoop).toMatchRenderedOutput(null); - - jest.advanceTimersByTime(1000); - expect(Scheduler).toHaveYielded(['Promise resolved [B]']); - expect(Scheduler).toFlushAndYield(['A', 'B', 'C']); - expect(ReactNoop).toMatchRenderedOutput('ABC'); - - // Update - ReactNoop.render(); - expect(Scheduler).toFlushAndYield([ - 'A', - 'Suspend! [B2]', - 'C', - 'Loading...', - ]); - // Time out the update - jest.advanceTimersByTime(750); - expect(Scheduler).toFlushAndYield([]); - expect(ReactNoop).toMatchRenderedOutput('Loading...'); - - // Resolve the promise - jest.advanceTimersByTime(1000); - expect(Scheduler).toHaveYielded(['Promise resolved [B2]']); - expect(Scheduler).toFlushAndYield(['A', 'B2', 'C']); - - // Render the final update. A should still be hidden, because it was - // given a `hidden` prop. - expect(ReactNoop).toMatchRenderedOutput('AB2C'); - }); + // Initial mount + ReactNoop.render(); + + expect(Scheduler).toFlushAndYield(['a', 'Suspend! [b]', 'c', 'Loading...']); + + expect(ReactNoop).toMatchRenderedOutput(null); + + jest.advanceTimersByTime(1000); + expect(Scheduler).toHaveYielded(['Promise resolved [b]']); + expect(Scheduler).toFlushAndYield(['a', 'b', 'c']); + expect(ReactNoop).toMatchRenderedOutput(ABC); + + // Update + ReactNoop.render(); + expect(Scheduler).toFlushAndYield([ + 'a', + 'Suspend! [b2]', + 'c', + 'Loading...', + ]); + // Time out the update + jest.advanceTimersByTime(750); + expect(Scheduler).toFlushAndYield([]); + expect(ReactNoop).toMatchRenderedOutput(LOADING...); + + // Resolve the promise + jest.advanceTimersByTime(1000); + expect(Scheduler).toHaveYielded(['Promise resolved [b2]']); + expect(Scheduler).toFlushAndYield(['a', 'b2', 'c']); + + // Render the final update. A should still be hidden, because it was + // given a `hidden` prop. + expect(ReactNoop).toMatchRenderedOutput(AB2C); + }); + + describe('profiler durations', () => { + let App; + let onRender; - it('preserves host context for text nodes', () => { - function App(props) { + beforeEach(() => { + // Order of parameters: id, phase, actualDuration, treeBaseDuration + onRender = jest.fn(); + + const Fallback = () => { + Scheduler.yieldValue('Fallback'); + Scheduler.advanceTime(10); + return 'Loading...'; + }; + + const Suspending = () => { + Scheduler.yieldValue('Suspending'); + Scheduler.advanceTime(2); + return ; + }; + + App = ({shouldSuspend, text = 'Text', textRenderDuration = 5}) => { + Scheduler.yieldValue('App'); return ( - // uppercase is a special type that causes React Noop to render child - // text nodes as uppercase. - - }> - - - + + }> + {shouldSuspend && } + - + ); - } - - // Initial mount - ReactNoop.render(); - - expect(Scheduler).toFlushAndYield([ - 'a', - 'Suspend! [b]', - 'c', - 'Loading...', - ]); - - expect(ReactNoop).toMatchRenderedOutput(null); - - jest.advanceTimersByTime(1000); - expect(Scheduler).toHaveYielded(['Promise resolved [b]']); - expect(Scheduler).toFlushAndYield(['a', 'b', 'c']); - expect(ReactNoop).toMatchRenderedOutput(ABC); - - // Update - ReactNoop.render(); - expect(Scheduler).toFlushAndYield([ - 'a', - 'Suspend! [b2]', - 'c', - 'Loading...', - ]); - // Time out the update - jest.advanceTimersByTime(750); - expect(Scheduler).toFlushAndYield([]); - expect(ReactNoop).toMatchRenderedOutput( - LOADING..., - ); - - // Resolve the promise - jest.advanceTimersByTime(1000); - expect(Scheduler).toHaveYielded(['Promise resolved [b2]']); - expect(Scheduler).toFlushAndYield(['a', 'b2', 'c']); - - // Render the final update. A should still be hidden, because it was - // given a `hidden` prop. - expect(ReactNoop).toMatchRenderedOutput(AB2C); + }; }); - describe('profiler durations', () => { - let App; - let onRender; - - beforeEach(() => { - // Order of parameters: id, phase, actualDuration, treeBaseDuration - onRender = jest.fn(); - - const Fallback = () => { - Scheduler.yieldValue('Fallback'); - Scheduler.advanceTime(10); - return 'Loading...'; - }; - - const Suspending = () => { - Scheduler.yieldValue('Suspending'); - Scheduler.advanceTime(2); - return ; - }; - - App = ({shouldSuspend, text = 'Text', textRenderDuration = 5}) => { - Scheduler.yieldValue('App'); - return ( - - }> - {shouldSuspend && } - - - - ); - }; + describe('when suspending during mount', () => { + it('properly accounts for base durations when a suspended times out in a sync tree', () => { + ReactNoop.renderLegacySyncRoot(); + expect(Scheduler).toHaveYielded([ + 'App', + 'Suspending', + 'Suspend! [Loaded]', + 'Text', + 'Fallback', + ]); + expect(ReactNoop).toMatchRenderedOutput('Loading...'); + expect(onRender).toHaveBeenCalledTimes(1); + + // Initial mount only shows the "Loading..." Fallback. + // The treeBaseDuration then should be 10ms spent rendering Fallback, + // but the actualDuration should also include the 8ms spent rendering the hidden tree. + expect(onRender.mock.calls[0][2]).toBe(18); + expect(onRender.mock.calls[0][3]).toBe(10); + + jest.advanceTimersByTime(1000); + + expect(Scheduler).toHaveYielded([ + 'Promise resolved [Loaded]', + 'Loaded', + ]); + expect(ReactNoop).toMatchRenderedOutput('LoadedText'); + expect(onRender).toHaveBeenCalledTimes(2); + + // When the suspending data is resolved and our final UI is rendered, + // the baseDuration should only include the 1ms re-rendering AsyncText, + // but the treeBaseDuration should include the full 8ms spent in the tree. + expect(onRender.mock.calls[1][2]).toBe(1); + expect(onRender.mock.calls[1][3]).toBe(8); }); - describe('when suspending during mount', () => { - it('properly accounts for base durations when a suspended times out in a sync tree', () => { - ReactNoop.renderLegacySyncRoot(); - expect(Scheduler).toHaveYielded([ - 'App', - 'Suspending', - 'Suspend! [Loaded]', - 'Text', - 'Fallback', - ]); - expect(ReactNoop).toMatchRenderedOutput('Loading...'); - expect(onRender).toHaveBeenCalledTimes(1); - - // Initial mount only shows the "Loading..." Fallback. - // The treeBaseDuration then should be 10ms spent rendering Fallback, - // but the actualDuration should also include the 8ms spent rendering the hidden tree. - expect(onRender.mock.calls[0][2]).toBe(18); - expect(onRender.mock.calls[0][3]).toBe(10); - - jest.advanceTimersByTime(1000); - - expect(Scheduler).toHaveYielded([ - 'Promise resolved [Loaded]', - 'Loaded', - ]); - expect(ReactNoop).toMatchRenderedOutput('LoadedText'); - expect(onRender).toHaveBeenCalledTimes(2); - - // When the suspending data is resolved and our final UI is rendered, - // the baseDuration should only include the 1ms re-rendering AsyncText, - // but the treeBaseDuration should include the full 8ms spent in the tree. - expect(onRender.mock.calls[1][2]).toBe(1); - expect(onRender.mock.calls[1][3]).toBe(8); - }); - - it('properly accounts for base durations when a suspended times out in a concurrent tree', () => { - ReactNoop.render(); - - expect(Scheduler).toFlushAndYield([ - 'App', - 'Suspending', - 'Suspend! [Loaded]', - 'Text', - 'Fallback', - ]); - expect(ReactNoop).toMatchRenderedOutput(null); - - // Show the fallback UI. - jest.advanceTimersByTime(750); - expect(ReactNoop).toMatchRenderedOutput('Loading...'); - expect(onRender).toHaveBeenCalledTimes(1); - - // Initial mount only shows the "Loading..." Fallback. - // The treeBaseDuration then should be 10ms spent rendering Fallback, - // but the actualDuration should also include the 8ms spent rendering the hidden tree. - expect(onRender.mock.calls[0][2]).toBe(18); - expect(onRender.mock.calls[0][3]).toBe(10); - - // Resolve the pending promise. - jest.advanceTimersByTime(250); - expect(Scheduler).toHaveYielded(['Promise resolved [Loaded]']); - expect(Scheduler).toFlushAndYield(['Suspending', 'Loaded', 'Text']); - expect(ReactNoop).toMatchRenderedOutput('LoadedText'); - expect(onRender).toHaveBeenCalledTimes(2); - - // When the suspending data is resolved and our final UI is rendered, - // both times should include the 8ms re-rendering Suspending and AsyncText. - expect(onRender.mock.calls[1][2]).toBe(8); - expect(onRender.mock.calls[1][3]).toBe(8); - }); + it('properly accounts for base durations when a suspended times out in a concurrent tree', () => { + ReactNoop.render(); + + expect(Scheduler).toFlushAndYield([ + 'App', + 'Suspending', + 'Suspend! [Loaded]', + 'Text', + 'Fallback', + ]); + expect(ReactNoop).toMatchRenderedOutput(null); + + // Show the fallback UI. + jest.advanceTimersByTime(750); + expect(ReactNoop).toMatchRenderedOutput('Loading...'); + expect(onRender).toHaveBeenCalledTimes(1); + + // Initial mount only shows the "Loading..." Fallback. + // The treeBaseDuration then should be 10ms spent rendering Fallback, + // but the actualDuration should also include the 8ms spent rendering the hidden tree. + expect(onRender.mock.calls[0][2]).toBe(18); + expect(onRender.mock.calls[0][3]).toBe(10); + + // Resolve the pending promise. + jest.advanceTimersByTime(250); + expect(Scheduler).toHaveYielded(['Promise resolved [Loaded]']); + expect(Scheduler).toFlushAndYield(['Suspending', 'Loaded', 'Text']); + expect(ReactNoop).toMatchRenderedOutput('LoadedText'); + expect(onRender).toHaveBeenCalledTimes(2); + + // When the suspending data is resolved and our final UI is rendered, + // both times should include the 8ms re-rendering Suspending and AsyncText. + expect(onRender.mock.calls[1][2]).toBe(8); + expect(onRender.mock.calls[1][3]).toBe(8); + }); + }); + + describe('when suspending during update', () => { + it('properly accounts for base durations when a suspended times out in a sync tree', () => { + ReactNoop.renderLegacySyncRoot( + , + ); + expect(Scheduler).toHaveYielded(['App', 'Text']); + expect(ReactNoop).toMatchRenderedOutput('Text'); + expect(onRender).toHaveBeenCalledTimes(1); + + // Initial mount only shows the "Text" text. + // It should take 5ms to render. + expect(onRender.mock.calls[0][2]).toBe(5); + expect(onRender.mock.calls[0][3]).toBe(5); + + ReactNoop.render(); + expect(Scheduler).toHaveYielded([ + 'App', + 'Suspending', + 'Suspend! [Loaded]', + 'Text', + 'Fallback', + ]); + expect(ReactNoop).toMatchRenderedOutput('Loading...'); + expect(onRender).toHaveBeenCalledTimes(2); + + // The suspense update should only show the "Loading..." Fallback. + // Both durations should include 10ms spent rendering Fallback + // plus the 8ms rendering the (hidden) components. + expect(onRender.mock.calls[1][2]).toBe(18); + expect(onRender.mock.calls[1][3]).toBe(18); + + ReactNoop.renderLegacySyncRoot( + , + ); + expect(Scheduler).toHaveYielded([ + 'App', + 'Suspending', + 'Suspend! [Loaded]', + 'New', + 'Fallback', + ]); + expect(ReactNoop).toMatchRenderedOutput('Loading...'); + expect(onRender).toHaveBeenCalledTimes(3); + + // If we force another update while still timed out, + // but this time the Text component took 1ms longer to render. + // This should impact both actualDuration and treeBaseDuration. + expect(onRender.mock.calls[2][2]).toBe(19); + expect(onRender.mock.calls[2][3]).toBe(19); + + jest.advanceTimersByTime(1000); + + expect(Scheduler).toHaveYielded([ + 'Promise resolved [Loaded]', + 'Loaded', + ]); + expect(ReactNoop).toMatchRenderedOutput('LoadedNew'); + expect(onRender).toHaveBeenCalledTimes(4); + + // When the suspending data is resolved and our final UI is rendered, + // the baseDuration should only include the 1ms re-rendering AsyncText, + // but the treeBaseDuration should include the full 9ms spent in the tree. + expect(onRender.mock.calls[3][2]).toBe(1); + expect(onRender.mock.calls[3][3]).toBe(9); }); - describe('when suspending during update', () => { - it('properly accounts for base durations when a suspended times out in a sync tree', () => { - ReactNoop.renderLegacySyncRoot( - , - ); - expect(Scheduler).toHaveYielded(['App', 'Text']); - expect(ReactNoop).toMatchRenderedOutput('Text'); - expect(onRender).toHaveBeenCalledTimes(1); - - // Initial mount only shows the "Text" text. - // It should take 5ms to render. - expect(onRender.mock.calls[0][2]).toBe(5); - expect(onRender.mock.calls[0][3]).toBe(5); - - ReactNoop.render(); - expect(Scheduler).toHaveYielded([ - 'App', - 'Suspending', - 'Suspend! [Loaded]', - 'Text', - 'Fallback', - ]); - expect(ReactNoop).toMatchRenderedOutput('Loading...'); - expect(onRender).toHaveBeenCalledTimes(2); - - // The suspense update should only show the "Loading..." Fallback. - // Both durations should include 10ms spent rendering Fallback - // plus the 8ms rendering the (hidden) components. - expect(onRender.mock.calls[1][2]).toBe(18); - expect(onRender.mock.calls[1][3]).toBe(18); - - ReactNoop.renderLegacySyncRoot( - , - ); - expect(Scheduler).toHaveYielded([ - 'App', - 'Suspending', - 'Suspend! [Loaded]', - 'New', - 'Fallback', - ]); - expect(ReactNoop).toMatchRenderedOutput('Loading...'); - expect(onRender).toHaveBeenCalledTimes(3); - - // If we force another update while still timed out, - // but this time the Text component took 1ms longer to render. - // This should impact both actualDuration and treeBaseDuration. - expect(onRender.mock.calls[2][2]).toBe(19); - expect(onRender.mock.calls[2][3]).toBe(19); - - jest.advanceTimersByTime(1000); - - expect(Scheduler).toHaveYielded([ - 'Promise resolved [Loaded]', - 'Loaded', - ]); - expect(ReactNoop).toMatchRenderedOutput('LoadedNew'); - expect(onRender).toHaveBeenCalledTimes(4); - - // When the suspending data is resolved and our final UI is rendered, - // the baseDuration should only include the 1ms re-rendering AsyncText, - // but the treeBaseDuration should include the full 9ms spent in the tree. - expect(onRender.mock.calls[3][2]).toBe(1); - expect(onRender.mock.calls[3][3]).toBe(9); - }); - - it('properly accounts for base durations when a suspended times out in a concurrent tree', () => { - ReactNoop.render( - , - ); - - expect(Scheduler).toFlushAndYield(['App', 'Text']); - expect(ReactNoop).toMatchRenderedOutput('Text'); - expect(onRender).toHaveBeenCalledTimes(1); - - // Initial mount only shows the "Text" text. - // It should take 5ms to render. - expect(onRender.mock.calls[0][2]).toBe(5); - expect(onRender.mock.calls[0][3]).toBe(5); - - ReactNoop.render(); - expect(Scheduler).toFlushAndYield([ - 'App', - 'Suspending', - 'Suspend! [Loaded]', - 'Text', - 'Fallback', - ]); - expect(ReactNoop).toMatchRenderedOutput('Text'); - - // Show the fallback UI. - jest.advanceTimersByTime(750); - expect(ReactNoop).toMatchRenderedOutput('Loading...'); - expect(onRender).toHaveBeenCalledTimes(2); - - // The suspense update should only show the "Loading..." Fallback. - // The actual duration should include 10ms spent rendering Fallback, - // plus the 8ms render all of the hidden, suspended subtree. - // But the tree base duration should only include 10ms spent rendering Fallback, - // plus the 5ms rendering the previously committed version of the hidden tree. - expect(onRender.mock.calls[1][2]).toBe(18); - expect(onRender.mock.calls[1][3]).toBe(15); - - // Update again while timed out. - ReactNoop.render( - , - ); - expect(Scheduler).toFlushAndYield([ - 'App', - 'Suspending', - 'Suspend! [Loaded]', - 'New', - 'Fallback', - ]); - expect(ReactNoop).toMatchRenderedOutput('Loading...'); - expect(onRender).toHaveBeenCalledTimes(2); - - // Resolve the pending promise. - jest.advanceTimersByTime(250); - expect(Scheduler).toHaveYielded(['Promise resolved [Loaded]']); - expect(Scheduler).toFlushAndYield([ - 'App', - 'Suspending', - 'Loaded', - 'New', - ]); - expect(onRender).toHaveBeenCalledTimes(3); - - // When the suspending data is resolved and our final UI is rendered, - // both times should include the 6ms rendering Text, - // the 2ms rendering Suspending, and the 1ms rendering AsyncText. - expect(onRender.mock.calls[2][2]).toBe(9); - expect(onRender.mock.calls[2][3]).toBe(9); - }); + it('properly accounts for base durations when a suspended times out in a concurrent tree', () => { + ReactNoop.render(); + + expect(Scheduler).toFlushAndYield(['App', 'Text']); + expect(ReactNoop).toMatchRenderedOutput('Text'); + expect(onRender).toHaveBeenCalledTimes(1); + + // Initial mount only shows the "Text" text. + // It should take 5ms to render. + expect(onRender.mock.calls[0][2]).toBe(5); + expect(onRender.mock.calls[0][3]).toBe(5); + + ReactNoop.render(); + expect(Scheduler).toFlushAndYield([ + 'App', + 'Suspending', + 'Suspend! [Loaded]', + 'Text', + 'Fallback', + ]); + expect(ReactNoop).toMatchRenderedOutput('Text'); + + // Show the fallback UI. + jest.advanceTimersByTime(750); + expect(ReactNoop).toMatchRenderedOutput('Loading...'); + expect(onRender).toHaveBeenCalledTimes(2); + + // The suspense update should only show the "Loading..." Fallback. + // The actual duration should include 10ms spent rendering Fallback, + // plus the 8ms render all of the hidden, suspended subtree. + // But the tree base duration should only include 10ms spent rendering Fallback, + // plus the 5ms rendering the previously committed version of the hidden tree. + expect(onRender.mock.calls[1][2]).toBe(18); + expect(onRender.mock.calls[1][3]).toBe(15); + + // Update again while timed out. + ReactNoop.render( + , + ); + expect(Scheduler).toFlushAndYield([ + 'App', + 'Suspending', + 'Suspend! [Loaded]', + 'New', + 'Fallback', + ]); + expect(ReactNoop).toMatchRenderedOutput('Loading...'); + expect(onRender).toHaveBeenCalledTimes(2); + + // Resolve the pending promise. + jest.advanceTimersByTime(250); + expect(Scheduler).toHaveYielded(['Promise resolved [Loaded]']); + expect(Scheduler).toFlushAndYield([ + 'App', + 'Suspending', + 'Loaded', + 'New', + ]); + expect(onRender).toHaveBeenCalledTimes(3); + + // When the suspending data is resolved and our final UI is rendered, + // both times should include the 6ms rendering Text, + // the 2ms rendering Suspending, and the 1ms rendering AsyncText. + expect(onRender.mock.calls[2][2]).toBe(9); + expect(onRender.mock.calls[2][3]).toBe(9); }); }); }); -} +}); diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js index 6e8727d0c64ef..abd5fe012e213 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js @@ -1387,44 +1387,47 @@ describe('ReactSuspenseWithNoopRenderer', () => { expect(ReactNoop.getChildren()).toEqual([span('Hi')]); }); - it('toggles visibility during the mutation phase', async () => { - const {useRef, useLayoutEffect} = React; + if (!global.__PERSISTENT__) { + // TODO: Write persistent version of this test + it('toggles visibility during the mutation phase', async () => { + const {useRef, useLayoutEffect} = React; - function Parent() { - const child = useRef(null); + function Parent() { + const child = useRef(null); - useLayoutEffect(() => { - Scheduler.yieldValue('Child is hidden: ' + child.current.hidden); - }); + useLayoutEffect(() => { + Scheduler.yieldValue('Child is hidden: ' + child.current.hidden); + }); - return ( - - ); - } + return ( + + ); + } - function App(props) { - return ( - }> - - - ); - } + function App(props) { + return ( + }> + + + ); + } - ReactNoop.renderLegacySyncRoot(); + ReactNoop.renderLegacySyncRoot(); - expect(Scheduler).toHaveYielded([ - 'Suspend! [Hi]', - 'Loading...', - // The child should have already been hidden - 'Child is hidden: true', - ]); + expect(Scheduler).toHaveYielded([ + 'Suspend! [Hi]', + 'Loading...', + // The child should have already been hidden + 'Child is hidden: true', + ]); - await advanceTimers(1000); + await advanceTimers(1000); - expect(Scheduler).toHaveYielded(['Promise resolved [Hi]', 'Hi']); - }); + expect(Scheduler).toHaveYielded(['Promise resolved [Hi]', 'Hi']); + }); + } }); it('does not call lifecycles of a suspended component', async () => { diff --git a/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js b/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js index 4a68193c8534d..c1f03e1085b1b 100644 --- a/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js +++ b/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js @@ -91,10 +91,7 @@ export const finalizeContainerChildren = $$$hostConfig.finalizeContainerChildren; export const replaceContainerChildren = $$$hostConfig.replaceContainerChildren; export const cloneHiddenInstance = $$$hostConfig.cloneHiddenInstance; -export const cloneUnhiddenInstance = $$$hostConfig.cloneUnhiddenInstance; export const cloneHiddenTextInstance = $$$hostConfig.cloneHiddenTextInstance; -export const cloneUnhiddenTextInstance = - $$$hostConfig.cloneUnhiddenTextInstance; // ------------------- // Hydration diff --git a/packages/shared/HostConfigWithNoPersistence.js b/packages/shared/HostConfigWithNoPersistence.js index ffd8342708e34..d5f84cf43fd6d 100644 --- a/packages/shared/HostConfigWithNoPersistence.js +++ b/packages/shared/HostConfigWithNoPersistence.js @@ -29,6 +29,4 @@ export const appendChildToContainerChildSet = shim; export const finalizeContainerChildren = shim; export const replaceContainerChildren = shim; export const cloneHiddenInstance = shim; -export const cloneUnhiddenInstance = shim; export const cloneHiddenTextInstance = shim; -export const cloneUnhiddenTextInstance = shim; diff --git a/scripts/circleci/test_entry_point.sh b/scripts/circleci/test_entry_point.sh index 5b46ec45d3b6e..b190ce672d2cf 100755 --- a/scripts/circleci/test_entry_point.sh +++ b/scripts/circleci/test_entry_point.sh @@ -11,6 +11,7 @@ if [ $((0 % CIRCLE_NODE_TOTAL)) -eq "$CIRCLE_NODE_INDEX" ]; then COMMANDS_TO_RUN+=('node ./scripts/tasks/flow-ci') COMMANDS_TO_RUN+=('node ./scripts/tasks/eslint') COMMANDS_TO_RUN+=('yarn test --maxWorkers=2') + COMMANDS_TO_RUN+=('yarn test-persistent --maxWorkers=2') COMMANDS_TO_RUN+=('./scripts/circleci/check_license.sh') COMMANDS_TO_RUN+=('./scripts/circleci/check_modules.sh') COMMANDS_TO_RUN+=('./scripts/circleci/test_print_warnings.sh') diff --git a/scripts/jest/config.source-persistent.js b/scripts/jest/config.source-persistent.js new file mode 100644 index 0000000000000..df6750d3f5f73 --- /dev/null +++ b/scripts/jest/config.source-persistent.js @@ -0,0 +1,18 @@ +'use strict'; + +const baseConfig = require('./config.base'); + +module.exports = Object.assign({}, baseConfig, { + modulePathIgnorePatterns: [ + 'ReactIncrementalPerf', + 'ReactIncrementalUpdatesMinimalism', + 'ReactIncrementalTriangle', + 'ReactIncrementalReflection', + 'forwardRef', + ], + setupFiles: [ + ...baseConfig.setupFiles, + require.resolve('./setupTests.persistent.js'), + require.resolve('./setupHostConfigs.js'), + ], +}); diff --git a/scripts/jest/setupTests.persistent.js b/scripts/jest/setupTests.persistent.js new file mode 100644 index 0000000000000..e07e74e456e02 --- /dev/null +++ b/scripts/jest/setupTests.persistent.js @@ -0,0 +1,7 @@ +'use strict'; + +jest.mock('react-noop-renderer', () => + require.requireActual('react-noop-renderer/persistent') +); + +global.__PERSISTENT__ = true; From 6a4a261ee8cf2f95c7a725670dad7763836d8284 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 11 Mar 2019 11:19:20 -0700 Subject: [PATCH 14/22] Test suspended children are hidden before layout in persistent mode (#15030) Refs behave differently in persistent mode, so instead of a ref, the persistent mode version of this test asserts on the output of the host tree. --- .../src/createReactNoop.js | 37 +++++++++++++-- ...tSuspenseWithNoopRenderer-test.internal.js | 47 +++++++++++++++++-- 2 files changed, 78 insertions(+), 6 deletions(-) diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index f3872ac483b57..d7cfef9cc7648 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -32,6 +32,7 @@ type Thenable = { type Container = { rootID: string, children: Array, + pendingChildren: Array, }; type Props = {prop: any, hidden: boolean, children?: mixed}; type Instance = {| @@ -457,7 +458,9 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { finalizeContainerChildren( container: Container, newChildren: Array, - ): void {}, + ): void { + container.pendingChildren = newChildren; + }, replaceContainerChildren( container: Container, @@ -581,13 +584,22 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { } }, + getPendingChildren(rootID: string = DEFAULT_ROOT_ID) { + const container = rootContainers.get(rootID); + if (container) { + return container.pendingChildren; + } else { + return null; + } + }, + getOrCreateRootContainer( rootID: string = DEFAULT_ROOT_ID, isConcurrent: boolean = false, ) { let root = roots.get(rootID); if (!root) { - const container = {rootID: rootID, children: []}; + const container = {rootID: rootID, pendingChildren: [], children: []}; rootContainers.set(rootID, container); root = NoopRenderer.createContainer(container, isConcurrent, false); roots.set(rootID, root); @@ -614,6 +626,25 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { return children; }, + getPendingChildrenAsJSX(rootID: string = DEFAULT_ROOT_ID) { + const children = childToJSX(ReactNoop.getPendingChildren(rootID), null); + if (children === null) { + return null; + } + if (Array.isArray(children)) { + return { + $$typeof: REACT_ELEMENT_TYPE, + type: REACT_FRAGMENT_TYPE, + key: null, + ref: null, + props: {children}, + _owner: null, + _store: __DEV__ ? {} : undefined, + }; + } + return children; + }, + createPortal( children: ReactNodeList, container: Container, @@ -778,7 +809,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { // Trick to flush passive effects without exposing an internal API: // Create a throwaway root and schedule a dummy update on it. const rootID = 'bloopandthenmoreletterstoavoidaconflict'; - const container = {rootID: rootID, children: []}; + const container = {rootID: rootID, pendingChildren: [], children: []}; rootContainers.set(rootID, container); const root = NoopRenderer.createContainer(container, true, false); NoopRenderer.updateContainer(null, root, null, null); diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js index abd5fe012e213..f438dbb2dafd4 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js @@ -1387,9 +1387,50 @@ describe('ReactSuspenseWithNoopRenderer', () => { expect(ReactNoop.getChildren()).toEqual([span('Hi')]); }); - if (!global.__PERSISTENT__) { - // TODO: Write persistent version of this test - it('toggles visibility during the mutation phase', async () => { + if (global.__PERSISTENT__) { + it('hides/unhides suspended children before layout effects fire (persistent)', async () => { + const {useRef, useLayoutEffect} = React; + + function Parent() { + const child = useRef(null); + + useLayoutEffect(() => { + Scheduler.yieldValue(ReactNoop.getPendingChildrenAsJSX()); + }); + + return ( + + ); + } + + function App(props) { + return ( + }> + + + ); + } + + ReactNoop.renderLegacySyncRoot(); + + expect(Scheduler).toHaveYielded([ + 'Suspend! [Hi]', + 'Loading...', + // The child should have already been hidden + + , + ]); + + await advanceTimers(1000); + + expect(Scheduler).toHaveYielded(['Promise resolved [Hi]', 'Hi']); + }); + } else { + it('hides/unhides suspended children before layout effects fire (mutation)', async () => { const {useRef, useLayoutEffect} = React; function Parent() { From 5d0c3c6c7d0a1f8fe793ca89d3e0f9f71f3e49a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Mon, 11 Mar 2019 13:50:19 -0700 Subject: [PATCH 15/22] [Partial Hydration] Render client-only content at normal priority (#15061) * Split props changing from permanent fallback state These will need different logic. In this commit, no logic has changed, only moved. * Delete terminal fallback content in first pass If the dehydrated suspense boundary's fallback content is terminal there is nothing to show. We need to get actual content on the screen soon. If we deprioritize that work to offscreen, then the timeout heuristics will be wrong. Therefore, if we have no current and we're already at terminal fallback state we'll immediately schedule a deletion and upgrade to real suspense. * Show failing case when there is another wrapper boundary * Revert "Delete terminal fallback content in first pass" This reverts commit ad67ba8928c23f5d9ba22d7e5c202bf27d0e49d3. * Use the new approach of leaving work at normal pri to replace fallback --- ...DOMServerPartialHydration-test.internal.js | 120 ++++++++++++++++++ .../src/ReactFiberBeginWork.js | 120 ++++++++++++------ 2 files changed, 198 insertions(+), 42 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js index 22e5f30f00091..b0e62e2fc789c 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js @@ -703,6 +703,126 @@ describe('ReactDOMServerPartialHydration', () => { expect(ref.current).toBe(span); }); + it('replaces the fallback within the maxDuration if there is a nested suspense', async () => { + let suspend = false; + let promise = new Promise(resolvePromise => {}); + let ref = React.createRef(); + + function Child() { + if (suspend) { + throw promise; + } else { + return 'Hello'; + } + } + + function InnerChild() { + // Always suspends indefinitely + throw promise; + } + + function App() { + return ( +
+ + + + + + + + +
+ ); + } + + // First we render the final HTML. With the streaming renderer + // this may have suspense points on the server but here we want + // to test the completed HTML. Don't suspend on the server. + suspend = true; + let finalHTML = ReactDOMServer.renderToString(); + let container = document.createElement('div'); + container.innerHTML = finalHTML; + + expect(container.getElementsByTagName('span').length).toBe(0); + + // On the client we have the data available quickly for some reason. + suspend = false; + let root = ReactDOM.unstable_createRoot(container, {hydrate: true}); + root.render(); + Scheduler.flushAll(); + // This will have exceeded the maxDuration so we should timeout. + jest.advanceTimersByTime(500); + // The boundary should longer be suspended for the middle content + // even though the inner boundary is still suspended. + + expect(container.textContent).toBe('Hello'); + + let span = container.getElementsByTagName('span')[0]; + expect(ref.current).toBe(span); + }); + + it('replaces the fallback within the maxDuration if there is a nested suspense in a nested suspense', async () => { + let suspend = false; + let promise = new Promise(resolvePromise => {}); + let ref = React.createRef(); + + function Child() { + if (suspend) { + throw promise; + } else { + return 'Hello'; + } + } + + function InnerChild() { + // Always suspends indefinitely + throw promise; + } + + function App() { + return ( +
+ + + + + + + + + + +
+ ); + } + + // First we render the final HTML. With the streaming renderer + // this may have suspense points on the server but here we want + // to test the completed HTML. Don't suspend on the server. + suspend = true; + let finalHTML = ReactDOMServer.renderToString(); + let container = document.createElement('div'); + container.innerHTML = finalHTML; + + expect(container.getElementsByTagName('span').length).toBe(0); + + // On the client we have the data available quickly for some reason. + suspend = false; + let root = ReactDOM.unstable_createRoot(container, {hydrate: true}); + root.render(); + Scheduler.flushAll(); + // This will have exceeded the maxDuration so we should timeout. + jest.advanceTimersByTime(500); + // The boundary should longer be suspended for the middle content + // even though the inner boundary is still suspended. + + expect(container.textContent).toBe('Hello'); + + let span = container.getElementsByTagName('span')[0]; + expect(ref.current).toBe(span); + }); + it('waits for pending content to come in from the server and then hydrates it', async () => { let suspend = false; let promise = new Promise(resolvePromise => {}); diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index e615c98c9f16f..f1b65633324c6 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -74,7 +74,11 @@ import { cloneChildFibers, } from './ReactChildFiber'; import {processUpdateQueue} from './ReactUpdateQueue'; -import {NoWork, Never} from './ReactFiberExpirationTime'; +import { + NoWork, + Never, + computeAsyncExpiration, +} from './ReactFiberExpirationTime'; import { ConcurrentMode, NoContext, @@ -133,7 +137,7 @@ import { createWorkInProgress, isSimpleFunctionComponent, } from './ReactFiber'; -import {retryTimedOutBoundary} from './ReactFiberScheduler'; +import {requestCurrentTime, retryTimedOutBoundary} from './ReactFiberScheduler'; const ReactCurrentOwner = ReactSharedInternals.ReactCurrentOwner; @@ -1631,15 +1635,71 @@ function updateSuspenseComponent( return next; } +function retrySuspenseComponentWithoutHydrating( + current: Fiber, + workInProgress: Fiber, + renderExpirationTime: ExpirationTime, +) { + // Detach from the current dehydrated boundary. + current.alternate = null; + workInProgress.alternate = null; + + // Insert a deletion in the effect list. + let returnFiber = workInProgress.return; + invariant( + returnFiber !== null, + 'Suspense boundaries are never on the root. ' + + 'This is probably a bug in React.', + ); + const last = returnFiber.lastEffect; + if (last !== null) { + last.nextEffect = current; + returnFiber.lastEffect = current; + } else { + returnFiber.firstEffect = returnFiber.lastEffect = current; + } + current.nextEffect = null; + current.effectTag = Deletion; + + // Upgrade this work in progress to a real Suspense component. + workInProgress.tag = SuspenseComponent; + workInProgress.stateNode = null; + workInProgress.memoizedState = null; + // This is now an insertion. + workInProgress.effectTag |= Placement; + // Retry as a real Suspense component. + return updateSuspenseComponent(null, workInProgress, renderExpirationTime); +} + function updateDehydratedSuspenseComponent( current: Fiber | null, workInProgress: Fiber, renderExpirationTime: ExpirationTime, ) { + const suspenseInstance = (workInProgress.stateNode: SuspenseInstance); if (current === null) { // During the first pass, we'll bail out and not drill into the children. // Instead, we'll leave the content in place and try to hydrate it later. - workInProgress.expirationTime = Never; + if (isSuspenseInstanceFallback(suspenseInstance)) { + // This is a client-only boundary. Since we won't get any content from the server + // for this, we need to schedule that at a higher priority based on when it would + // have timed out. In theory we could render it in this pass but it would have the + // wrong priority associated with it and will prevent hydration of parent path. + // Instead, we'll leave work left on it to render it in a separate commit. + + // TODO This time should be the time at which the server rendered response that is + // a parent to this boundary was displayed. However, since we currently don't have + // a protocol to transfer that time, we'll just estimate it by using the current + // time. This will mean that Suspense timeouts are slightly shifted to later than + // they should be. + let serverDisplayTime = requestCurrentTime(); + // Schedule a normal pri update to render this content. + workInProgress.expirationTime = computeAsyncExpiration(serverDisplayTime); + } else { + // We'll continue hydrating the rest at offscreen priority since we'll already + // be showing the right content coming from the server, it is no rush. + workInProgress.expirationTime = Never; + } return null; } if ((workInProgress.effectTag & DidCapture) !== NoEffect) { @@ -1648,55 +1708,31 @@ function updateDehydratedSuspenseComponent( workInProgress.child = null; return null; } + if (isSuspenseInstanceFallback(suspenseInstance)) { + // This boundary is in a permanent fallback state. In this case, we'll never + // get an update and we'll never be able to hydrate the final content. Let's just try the + // client side render instead. + return retrySuspenseComponentWithoutHydrating( + current, + workInProgress, + renderExpirationTime, + ); + } // We use childExpirationTime to indicate that a child might depend on context, so if // any context has changed, we need to treat is as if the input might have changed. const hasContextChanged = current.childExpirationTime >= renderExpirationTime; - const suspenseInstance = (current.stateNode: SuspenseInstance); - if ( - didReceiveUpdate || - hasContextChanged || - isSuspenseInstanceFallback(suspenseInstance) - ) { + if (didReceiveUpdate || hasContextChanged) { // This boundary has changed since the first render. This means that we are now unable to // hydrate it. We might still be able to hydrate it using an earlier expiration time but // during this render we can't. Instead, we're going to delete the whole subtree and // instead inject a new real Suspense boundary to take its place, which may render content // or fallback. The real Suspense boundary will suspend for a while so we have some time // to ensure it can produce real content, but all state and pending events will be lost. - - // Alternatively, this boundary is in a permanent fallback state. In this case, we'll never - // get an update and we'll never be able to hydrate the final content. Let's just try the - // client side render instead. - - // Detach from the current dehydrated boundary. - current.alternate = null; - workInProgress.alternate = null; - - // Insert a deletion in the effect list. - let returnFiber = workInProgress.return; - invariant( - returnFiber !== null, - 'Suspense boundaries are never on the root. ' + - 'This is probably a bug in React.', + return retrySuspenseComponentWithoutHydrating( + current, + workInProgress, + renderExpirationTime, ); - const last = returnFiber.lastEffect; - if (last !== null) { - last.nextEffect = current; - returnFiber.lastEffect = current; - } else { - returnFiber.firstEffect = returnFiber.lastEffect = current; - } - current.nextEffect = null; - current.effectTag = Deletion; - - // Upgrade this work in progress to a real Suspense component. - workInProgress.tag = SuspenseComponent; - workInProgress.stateNode = null; - workInProgress.memoizedState = null; - // This is now an insertion. - workInProgress.effectTag |= Placement; - // Retry as a real Suspense component. - return updateSuspenseComponent(null, workInProgress, renderExpirationTime); } else if (isSuspenseInstancePending(suspenseInstance)) { // This component is still pending more data from the server, so we can't hydrate its // content. We treat it as if this component suspended itself. It might seem as if From d4a60a14de099feeb52edfe20d6c73621b91ade9 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 5 Mar 2019 13:03:18 -0800 Subject: [PATCH 16/22] Added useSubscription reference hook and tests --- packages/create-subscription/index.js | 1 + .../useSubscription-test.internal.js | 357 ++++++++++++++++++ .../src/useSubscription.js | 77 ++++ 3 files changed, 435 insertions(+) create mode 100644 packages/create-subscription/src/__tests__/useSubscription-test.internal.js create mode 100644 packages/create-subscription/src/useSubscription.js diff --git a/packages/create-subscription/index.js b/packages/create-subscription/index.js index 314587a1351b1..b6e16aaee8613 100644 --- a/packages/create-subscription/index.js +++ b/packages/create-subscription/index.js @@ -10,3 +10,4 @@ 'use strict'; export * from './src/createSubscription'; +export * from './src/useSubscription'; diff --git a/packages/create-subscription/src/__tests__/useSubscription-test.internal.js b/packages/create-subscription/src/__tests__/useSubscription-test.internal.js new file mode 100644 index 0000000000000..c39202e61f4f1 --- /dev/null +++ b/packages/create-subscription/src/__tests__/useSubscription-test.internal.js @@ -0,0 +1,357 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @emails react-core + */ + +'use strict'; + +let act; +let useSubscription; +let BehaviorSubject; +let React; +let ReactTestRenderer; +let Scheduler; +let ReplaySubject; + +describe('useSubscription', () => { + beforeEach(() => { + jest.resetModules(); + jest.mock('scheduler', () => require('scheduler/unstable_mock')); + + useSubscription = require('create-subscription').useSubscription; + React = require('react'); + ReactTestRenderer = require('react-test-renderer'); + Scheduler = require('scheduler'); + + act = ReactTestRenderer.act; + + BehaviorSubject = require('rxjs').BehaviorSubject; + ReplaySubject = require('rxjs').ReplaySubject; + }); + + function createBehaviorSubject(initialValue) { + const behaviorSubject = new BehaviorSubject(); + if (initialValue) { + behaviorSubject.next(initialValue); + } + return behaviorSubject; + } + + function createReplaySubject(initialValue) { + const replaySubject = new ReplaySubject(); + if (initialValue) { + replaySubject.next(initialValue); + } + return replaySubject; + } + + // Mimic createSubscription API to simplify testing + function createSubscription({getCurrentValue, subscribe}) { + return function Subscription({children, source}) { + const value = useSubscription( + React.useMemo(() => ({source, getCurrentValue, subscribe}), [source]), + ); + + return React.createElement(children, {value}); + }; + } + + it('supports basic subscription pattern', () => { + const Subscription = createSubscription({ + getCurrentValue: source => source.getValue(), + subscribe: (source, callback) => { + const subscription = source.subscribe(callback); + return () => subscription.unsubscribe(); + }, + }); + + const observable = createBehaviorSubject(); + const renderer = ReactTestRenderer.create( + + {({value = 'default'}) => { + Scheduler.yieldValue(value); + return null; + }} + , + {unstable_isConcurrent: true}, + ); + + // Updates while subscribed should re-render the child component + expect(Scheduler).toFlushAndYield(['default']); + act(() => observable.next(123)); + expect(Scheduler).toFlushAndYield([123]); + act(() => observable.next('abc')); + expect(Scheduler).toFlushAndYield(['abc']); + + // Unmounting the subscriber should remove listeners + renderer.update(
); + act(() => observable.next(456)); + expect(Scheduler).toFlushAndYield([]); + }); + + it('should support observable types like RxJS ReplaySubject', () => { + const Subscription = createSubscription({ + getCurrentValue: source => { + let currentValue; + source + .subscribe(value => { + currentValue = value; + }) + .unsubscribe(); + return currentValue; + }, + subscribe: (source, callback) => { + const subscription = source.subscribe(callback); + return () => subscription.unsubscribe; + }, + }); + + function render({value = 'default'}) { + Scheduler.yieldValue(value); + return null; + } + + let observable = createReplaySubject('initial'); + const renderer = ReactTestRenderer.create( + {render}, + {unstable_isConcurrent: true}, + ); + expect(Scheduler).toFlushAndYield(['initial']); + act(() => observable.next('updated')); + expect(Scheduler).toFlushAndYield(['updated']); + + Scheduler.flushAll(); + + // Unsetting the subscriber prop should reset subscribed values + observable = createReplaySubject(undefined); + renderer.update({render}); + expect(Scheduler).toFlushAndYield(['default']); + }); + + it('should unsubscribe from old subscribables and subscribe to new subscribables when props change', () => { + const Subscription = createSubscription({ + getCurrentValue: source => source.getValue(), + subscribe: (source, callback) => { + const subscription = source.subscribe(callback); + return () => subscription.unsubscribe(); + }, + }); + + function render({value = 'default'}) { + Scheduler.yieldValue(value); + return null; + } + + const observableA = createBehaviorSubject('a-0'); + const observableB = createBehaviorSubject('b-0'); + + const renderer = ReactTestRenderer.create( + {render}, + {unstable_isConcurrent: true}, + ); + + // Updates while subscribed should re-render the child component + expect(Scheduler).toFlushAndYield(['a-0']); + + // Unsetting the subscriber prop should reset subscribed values + renderer.update({render}); + expect(Scheduler).toFlushAndYield(['b-0']); + + // Updates to the old subscribable should not re-render the child component + act(() => observableA.next('a-1')); + expect(Scheduler).toFlushAndYield([]); + + // Updates to the bew subscribable should re-render the child component + act(() => observableB.next('b-1')); + expect(Scheduler).toFlushAndYield(['b-1']); + }); + + it('should ignore values emitted by a new subscribable until the commit phase', () => { + const log = []; + + function Child({value}) { + Scheduler.yieldValue('Child: ' + value); + return null; + } + + const Subscription = createSubscription({ + getCurrentValue: source => source.getValue(), + subscribe: (source, callback) => { + const subscription = source.subscribe(callback); + return () => subscription.unsubscribe(); + }, + }); + + class Parent extends React.Component { + state = {}; + + static getDerivedStateFromProps(nextProps, prevState) { + if (nextProps.observed !== prevState.observed) { + return { + observed: nextProps.observed, + }; + } + + return null; + } + + componentDidMount() { + log.push('Parent.componentDidMount'); + } + + componentDidUpdate() { + log.push('Parent.componentDidUpdate'); + } + + render() { + return ( + + {({value = 'default'}) => { + Scheduler.yieldValue('Subscriber: ' + value); + return ; + }} + + ); + } + } + + const observableA = createBehaviorSubject('a-0'); + const observableB = createBehaviorSubject('b-0'); + + const renderer = ReactTestRenderer.create( + , + {unstable_isConcurrent: true}, + ); + expect(Scheduler).toFlushAndYield(['Subscriber: a-0', 'Child: a-0']); + expect(log).toEqual(['Parent.componentDidMount']); + + // Start React update, but don't finish + renderer.update(); + expect(Scheduler).toFlushAndYieldThrough(['Subscriber: b-0']); + expect(log).toEqual(['Parent.componentDidMount']); + + // Emit some updates from the uncommitted subscribable + observableB.next('b-1'); + observableB.next('b-2'); + observableB.next('b-3'); + + // Update again + renderer.update(); + + // Flush everything and ensure that the correct subscribable is used + // We expect the last emitted update to be rendered (because of the commit phase value check) + // But the intermediate ones should be ignored, + // And the final rendered output should be the higher-priority observable. + expect(Scheduler).toFlushAndYield([ + 'Child: b-0', + 'Subscriber: b-3', + 'Child: b-3', + 'Subscriber: a-0', + 'Child: a-0', + ]); + expect(log).toEqual([ + 'Parent.componentDidMount', + 'Parent.componentDidUpdate', + 'Parent.componentDidUpdate', + ]); + }); + + it('should not drop values emitted between updates', () => { + const log = []; + + function Child({value}) { + Scheduler.yieldValue('Child: ' + value); + return null; + } + + const Subscription = createSubscription({ + getCurrentValue: source => source.getValue(), + subscribe: (source, callback) => { + const subscription = source.subscribe(callback); + return () => subscription.unsubscribe(); + }, + }); + + class Parent extends React.Component { + state = {}; + + static getDerivedStateFromProps(nextProps, prevState) { + if (nextProps.observed !== prevState.observed) { + return { + observed: nextProps.observed, + }; + } + + return null; + } + + componentDidMount() { + log.push('Parent.componentDidMount'); + } + + componentDidUpdate() { + log.push('Parent.componentDidUpdate'); + } + + render() { + return ( + + {({value = 'default'}) => { + Scheduler.yieldValue('Subscriber: ' + value); + return ; + }} + + ); + } + } + + const observableA = createBehaviorSubject('a-0'); + const observableB = createBehaviorSubject('b-0'); + + const renderer = ReactTestRenderer.create( + , + {unstable_isConcurrent: true}, + ); + expect(Scheduler).toFlushAndYield(['Subscriber: a-0', 'Child: a-0']); + expect(log).toEqual(['Parent.componentDidMount']); + + // Start React update, but don't finish + renderer.update(); + expect(Scheduler).toFlushAndYieldThrough(['Subscriber: b-0']); + expect(log).toEqual(['Parent.componentDidMount']); + + // Emit some updates from the old subscribable + act(() => observableA.next('a-1')); + act(() => observableA.next('a-2')); + + // Update again + renderer.update(); + + // Flush everything and ensure that the correct subscribable is used + // We expect the new subscribable to finish rendering, + // But then the updated values from the old subscribable should be used. + expect(Scheduler).toFlushAndYield([ + 'Child: b-0', + 'Subscriber: a-2', + 'Child: a-2', + ]); + expect(log).toEqual([ + 'Parent.componentDidMount', + 'Parent.componentDidUpdate', + 'Parent.componentDidUpdate', + ]); + + // Updates from the new subscribable should be ignored. + act(() => observableB.next('b-1')); + expect(Scheduler).toFlushAndYield([]); + expect(log).toEqual([ + 'Parent.componentDidMount', + 'Parent.componentDidUpdate', + 'Parent.componentDidUpdate', + ]); + }); +}); diff --git a/packages/create-subscription/src/useSubscription.js b/packages/create-subscription/src/useSubscription.js new file mode 100644 index 0000000000000..4cfcadb61913a --- /dev/null +++ b/packages/create-subscription/src/useSubscription.js @@ -0,0 +1,77 @@ +import {useEffect, useReducer, useState} from 'react'; + +export function useSubscription({ + // This is the thing being subscribed to (e.g. an observable, event dispatcher, etc). + source, + + // (Synchronously) returns the current value of our subscription source. + getCurrentValue, + + // This function is passed an event handler to attach to the subscription source. + // It should return an unsubscribe function that removes the handler. + subscribe, +}) { + // Read the current value from our subscription source. + // When this value changes, we'll schedule an update with React. + // It's important to also store the source itself so that we can check for staleness. + // (See the comment in checkForUpdates() below for more info.) + const [state, setState] = useState({ + source, + value: getCurrentValue(source), + }); + + // If the source has changed since our last render, schedule an update with its current value. + if (state.source !== source) { + setState({ + source, + value: getCurrentValue(source), + }); + } + + // It is important not to subscribe while rendering because this can lead to memory leaks. + // (Learn more at reactjs.org/docs/strict-mode.html#detecting-unexpected-side-effects) + // Instead, we wait until the commit phase to attach our handler. + // + // We intentionally use a passive effect (useEffect) rather than a synchronous one (useLayoutEffect) + // so that we don't stretch the commit phase. + // This also has an added benefit when multiple components are subscribed to the same source: + // It allows each of the event handlers to safely schedule work without potentially removing an another handler. + // (Learn more at https://codesandbox.io/s/k0yvr5970o) + useEffect( + () => { + const checkForUpdates = () => { + setState(state => { + // Ignore values from stale sources! + // Since we subscribe an unsubscribe in a passive effect, + // it's possible that this callback will be invoked for a stale (previous) source. + // This check avoids scheduling an update for htat stale source. + if (state.source !== source) { + return state; + } + + // Some subscription sources will auto-invoke the handler, even if the value hasn't changed. + // If the value hasn't changed, no update is needed. + // Return state as-is so React can bail out and avoid an unnecessary render. + const value = getCurrentValue(source); + if (state.value === value) { + return state; + } + + return { ...state, value }; + }); + }; + const unsubscribe = subscribe(source, checkForUpdates); + + // Because we're subscribing in a passive effect, + // it's possible that an update has occurred between render and our effect handler. + // Check for this and schedule an update if work has occurred. + checkForUpdates(); + + return () => unsubscribe(); + }, + [getCurrentValue, source, subscribe], + ); + + // Return the current value for our caller to use while rendering. + return state.value; +} From 6830db57ce829a2cbc3918897a72c01a01df7f63 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 5 Mar 2019 13:03:31 -0800 Subject: [PATCH 17/22] Fixed a race case in useSubscription. Converted tests from Noop to Test renderer. --- .../useSubscription-test.internal.js | 53 +++++++++++++++++++ .../src/useSubscription.js | 29 +++++++--- 2 files changed, 74 insertions(+), 8 deletions(-) diff --git a/packages/create-subscription/src/__tests__/useSubscription-test.internal.js b/packages/create-subscription/src/__tests__/useSubscription-test.internal.js index c39202e61f4f1..0a5fbd0a317fd 100644 --- a/packages/create-subscription/src/__tests__/useSubscription-test.internal.js +++ b/packages/create-subscription/src/__tests__/useSubscription-test.internal.js @@ -354,4 +354,57 @@ describe('useSubscription', () => { 'Parent.componentDidUpdate', ]); }); + + it('should guard against updates that happen after unmounting', () => { + const Subscription = createSubscription({ + getCurrentValue: source => source.getValue(), + subscribe: (source, callback) => { + return source.subscribe(callback); + }, + }); + + const eventHandler = { + _callbacks: [], + _value: true, + change(value) { + eventHandler._value = value; + const _callbacks = eventHandler._callbacks.slice(0); + _callbacks.forEach(callback => callback(value)); + }, + getValue() { + return eventHandler._value; + }, + subscribe(callback) { + eventHandler._callbacks.push(callback); + return () => { + eventHandler._callbacks.splice( + eventHandler._callbacks.indexOf(callback), + 1, + ); + }; + }, + }; + + eventHandler.subscribe(value => { + if (value === false) { + renderer.unmount(); + expect(Scheduler).toFlushAndYield([]); + } + }); + + const renderer = ReactTestRenderer.create( + + {({value}) => { + Scheduler.yieldValue(value); + return null; + }} + , + {unstable_isConcurrent: true}, + ); + + expect(Scheduler).toFlushAndYield([true]); + + // This event should unmount + eventHandler.change(false); + }); }); diff --git a/packages/create-subscription/src/useSubscription.js b/packages/create-subscription/src/useSubscription.js index 4cfcadb61913a..ee4078fe2c4c9 100644 --- a/packages/create-subscription/src/useSubscription.js +++ b/packages/create-subscription/src/useSubscription.js @@ -1,4 +1,4 @@ -import {useEffect, useReducer, useState} from 'react'; +import {useEffect, useState} from 'react'; export function useSubscription({ // This is the thing being subscribed to (e.g. an observable, event dispatcher, etc). @@ -39,25 +39,35 @@ export function useSubscription({ // (Learn more at https://codesandbox.io/s/k0yvr5970o) useEffect( () => { + let didUnsubscribe = false; + const checkForUpdates = () => { - setState(state => { + // It's possible that this callback will be invoked even after being unsubscribed, + // if it's removed as a result of an event/update from the source. + // In this case, React will log a DEV warning about an update from an unmounted component. + // We can avoid triggering that warning with this check. + if (didUnsubscribe) { + return; + } + + setState(prevState => { // Ignore values from stale sources! // Since we subscribe an unsubscribe in a passive effect, // it's possible that this callback will be invoked for a stale (previous) source. // This check avoids scheduling an update for htat stale source. - if (state.source !== source) { - return state; + if (prevState.source !== source) { + return prevState; } // Some subscription sources will auto-invoke the handler, even if the value hasn't changed. // If the value hasn't changed, no update is needed. // Return state as-is so React can bail out and avoid an unnecessary render. const value = getCurrentValue(source); - if (state.value === value) { - return state; + if (prevState.value === value) { + return prevState; } - return { ...state, value }; + return {...prevState, value}; }); }; const unsubscribe = subscribe(source, checkForUpdates); @@ -67,7 +77,10 @@ export function useSubscription({ // Check for this and schedule an update if work has occurred. checkForUpdates(); - return () => unsubscribe(); + return () => { + didUnsubscribe = true; + unsubscribe(); + }; }, [getCurrentValue, source, subscribe], ); From acbb4d4874f9583ab707e90f790e939b45a36c91 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 5 Mar 2019 13:03:57 -0800 Subject: [PATCH 18/22] Fixed an additional unmount edge case. Moved hook into temporary package. --- packages/create-subscription/index.js | 1 - packages/react-hooks/README.md | 3 +++ packages/react-hooks/index.js | 12 ++++++++++ packages/react-hooks/npm/index.js | 7 ++++++ packages/react-hooks/package.json | 24 +++++++++++++++++++ .../useSubscription-test.internal.js | 2 +- .../src/useSubscription.js | 0 7 files changed, 47 insertions(+), 2 deletions(-) create mode 100644 packages/react-hooks/README.md create mode 100644 packages/react-hooks/index.js create mode 100644 packages/react-hooks/npm/index.js create mode 100644 packages/react-hooks/package.json rename packages/{create-subscription => react-hooks}/src/__tests__/useSubscription-test.internal.js (99%) rename packages/{create-subscription => react-hooks}/src/useSubscription.js (100%) diff --git a/packages/create-subscription/index.js b/packages/create-subscription/index.js index b6e16aaee8613..314587a1351b1 100644 --- a/packages/create-subscription/index.js +++ b/packages/create-subscription/index.js @@ -10,4 +10,3 @@ 'use strict'; export * from './src/createSubscription'; -export * from './src/useSubscription'; diff --git a/packages/react-hooks/README.md b/packages/react-hooks/README.md new file mode 100644 index 0000000000000..68bb2c4a262e5 --- /dev/null +++ b/packages/react-hooks/README.md @@ -0,0 +1,3 @@ +# react-hooks + +Placeholder package. \ No newline at end of file diff --git a/packages/react-hooks/index.js b/packages/react-hooks/index.js new file mode 100644 index 0000000000000..f5030786a9639 --- /dev/null +++ b/packages/react-hooks/index.js @@ -0,0 +1,12 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow + */ + +'use strict'; + +export * from './src/useSubscription'; diff --git a/packages/react-hooks/npm/index.js b/packages/react-hooks/npm/index.js new file mode 100644 index 0000000000000..a09b8cb3c55f1 --- /dev/null +++ b/packages/react-hooks/npm/index.js @@ -0,0 +1,7 @@ +'use strict'; + +if (process.env.NODE_ENV === 'production') { + module.exports = require('./cjs/react-hooks.production.min.js'); +} else { + module.exports = require('./cjs/react-hooks.development.js'); +} diff --git a/packages/react-hooks/package.json b/packages/react-hooks/package.json new file mode 100644 index 0000000000000..e08e0eaf92b31 --- /dev/null +++ b/packages/react-hooks/package.json @@ -0,0 +1,24 @@ +{ + "private": true, + "name": "react-hooks", + "description": "Reusable hooks", + "version": "16.8.3", + "repository": { + "type": "git", + "url": "https://github.com/facebook/react.git", + "directory": "packages/react-hooks" + }, + "files": [ + "LICENSE", + "README.md", + "build-info.json", + "index.js", + "cjs/" + ], + "peerDependencies": { + "react": "^16.3.0" + }, + "devDependencies": { + "rxjs": "^5.5.6" + } +} diff --git a/packages/create-subscription/src/__tests__/useSubscription-test.internal.js b/packages/react-hooks/src/__tests__/useSubscription-test.internal.js similarity index 99% rename from packages/create-subscription/src/__tests__/useSubscription-test.internal.js rename to packages/react-hooks/src/__tests__/useSubscription-test.internal.js index 0a5fbd0a317fd..3027a6c4c4466 100644 --- a/packages/create-subscription/src/__tests__/useSubscription-test.internal.js +++ b/packages/react-hooks/src/__tests__/useSubscription-test.internal.js @@ -22,7 +22,7 @@ describe('useSubscription', () => { jest.resetModules(); jest.mock('scheduler', () => require('scheduler/unstable_mock')); - useSubscription = require('create-subscription').useSubscription; + useSubscription = require('react-hooks').useSubscription; React = require('react'); ReactTestRenderer = require('react-test-renderer'); Scheduler = require('scheduler'); diff --git a/packages/create-subscription/src/useSubscription.js b/packages/react-hooks/src/useSubscription.js similarity index 100% rename from packages/create-subscription/src/useSubscription.js rename to packages/react-hooks/src/useSubscription.js From a3c83c7f5944f9064b53bdb2160d3020f9024c9f Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 5 Mar 2019 22:10:59 -0800 Subject: [PATCH 19/22] Updated useSubscription API to require deps array --- .../useSubscription-test.internal.js | 266 +++++++++--------- packages/react-hooks/src/useSubscription.js | 72 +++-- 2 files changed, 178 insertions(+), 160 deletions(-) diff --git a/packages/react-hooks/src/__tests__/useSubscription-test.internal.js b/packages/react-hooks/src/__tests__/useSubscription-test.internal.js index 3027a6c4c4466..6f3ed4225d1ca 100644 --- a/packages/react-hooks/src/__tests__/useSubscription-test.internal.js +++ b/packages/react-hooks/src/__tests__/useSubscription-test.internal.js @@ -49,34 +49,29 @@ describe('useSubscription', () => { return replaySubject; } - // Mimic createSubscription API to simplify testing - function createSubscription({getCurrentValue, subscribe}) { - return function Subscription({children, source}) { + it('supports basic subscription pattern', () => { + function Subscription({source}) { const value = useSubscription( - React.useMemo(() => ({source, getCurrentValue, subscribe}), [source]), + () => ({ + getCurrentValue: () => source.getValue(), + subscribe: callback => { + const subscription = source.subscribe(callback); + return () => subscription.unsubscribe(); + }, + }), + [source], ); + return ; + } - return React.createElement(children, {value}); - }; - } - - it('supports basic subscription pattern', () => { - const Subscription = createSubscription({ - getCurrentValue: source => source.getValue(), - subscribe: (source, callback) => { - const subscription = source.subscribe(callback); - return () => subscription.unsubscribe(); - }, - }); + function Child({value = 'default'}) { + Scheduler.yieldValue(value); + return null; + } const observable = createBehaviorSubject(); const renderer = ReactTestRenderer.create( - - {({value = 'default'}) => { - Scheduler.yieldValue(value); - return null; - }} - , + , {unstable_isConcurrent: true}, ); @@ -94,30 +89,36 @@ describe('useSubscription', () => { }); it('should support observable types like RxJS ReplaySubject', () => { - const Subscription = createSubscription({ - getCurrentValue: source => { - let currentValue; - source - .subscribe(value => { - currentValue = value; - }) - .unsubscribe(); - return currentValue; - }, - subscribe: (source, callback) => { - const subscription = source.subscribe(callback); - return () => subscription.unsubscribe; - }, - }); + function Subscription({source}) { + const value = useSubscription( + () => ({ + getCurrentValue: () => { + let currentValue; + source + .subscribe(tempValue => { + currentValue = tempValue; + }) + .unsubscribe(); + return currentValue; + }, + subscribe: callback => { + const subscription = source.subscribe(callback); + return () => subscription.unsubscribe(); + }, + }), + [source], + ); + return ; + } - function render({value = 'default'}) { + function Child({value = 'default'}) { Scheduler.yieldValue(value); return null; } let observable = createReplaySubject('initial'); const renderer = ReactTestRenderer.create( - {render}, + , {unstable_isConcurrent: true}, ); expect(Scheduler).toFlushAndYield(['initial']); @@ -128,20 +129,26 @@ describe('useSubscription', () => { // Unsetting the subscriber prop should reset subscribed values observable = createReplaySubject(undefined); - renderer.update({render}); + renderer.update(); expect(Scheduler).toFlushAndYield(['default']); }); it('should unsubscribe from old subscribables and subscribe to new subscribables when props change', () => { - const Subscription = createSubscription({ - getCurrentValue: source => source.getValue(), - subscribe: (source, callback) => { - const subscription = source.subscribe(callback); - return () => subscription.unsubscribe(); - }, - }); + function Subscription({source}) { + const value = useSubscription( + () => ({ + getCurrentValue: () => source.getValue(), + subscribe: callback => { + const subscription = source.subscribe(callback); + return () => subscription.unsubscribe(); + }, + }), + [source], + ); + return ; + } - function render({value = 'default'}) { + function Child({value = 'default'}) { Scheduler.yieldValue(value); return null; } @@ -150,7 +157,7 @@ describe('useSubscription', () => { const observableB = createBehaviorSubject('b-0'); const renderer = ReactTestRenderer.create( - {render}, + , {unstable_isConcurrent: true}, ); @@ -158,7 +165,7 @@ describe('useSubscription', () => { expect(Scheduler).toFlushAndYield(['a-0']); // Unsetting the subscriber prop should reset subscribed values - renderer.update({render}); + renderer.update(); expect(Scheduler).toFlushAndYield(['b-0']); // Updates to the old subscribable should not re-render the child component @@ -173,32 +180,31 @@ describe('useSubscription', () => { it('should ignore values emitted by a new subscribable until the commit phase', () => { const log = []; - function Child({value}) { - Scheduler.yieldValue('Child: ' + value); - return null; + function Subscription({source}) { + const value = useSubscription( + () => ({ + getCurrentValue: () => source.getValue(), + subscribe: callback => { + const subscription = source.subscribe(callback); + return () => subscription.unsubscribe(); + }, + }), + [source], + ); + return ; } - const Subscription = createSubscription({ - getCurrentValue: source => source.getValue(), - subscribe: (source, callback) => { - const subscription = source.subscribe(callback); - return () => subscription.unsubscribe(); - }, - }); - - class Parent extends React.Component { - state = {}; - - static getDerivedStateFromProps(nextProps, prevState) { - if (nextProps.observed !== prevState.observed) { - return { - observed: nextProps.observed, - }; - } + function Outer({value}) { + Scheduler.yieldValue('Outer: ' + value); + return ; + } - return null; - } + function Inner({value}) { + Scheduler.yieldValue('Inner: ' + value); + return null; + } + class Parent extends React.Component { componentDidMount() { log.push('Parent.componentDidMount'); } @@ -208,14 +214,7 @@ describe('useSubscription', () => { } render() { - return ( - - {({value = 'default'}) => { - Scheduler.yieldValue('Subscriber: ' + value); - return ; - }} - - ); + return ; } } @@ -226,12 +225,12 @@ describe('useSubscription', () => { , {unstable_isConcurrent: true}, ); - expect(Scheduler).toFlushAndYield(['Subscriber: a-0', 'Child: a-0']); + expect(Scheduler).toFlushAndYield(['Outer: a-0', 'Inner: a-0']); expect(log).toEqual(['Parent.componentDidMount']); // Start React update, but don't finish renderer.update(); - expect(Scheduler).toFlushAndYieldThrough(['Subscriber: b-0']); + expect(Scheduler).toFlushAndYieldThrough(['Outer: b-0']); expect(log).toEqual(['Parent.componentDidMount']); // Emit some updates from the uncommitted subscribable @@ -247,11 +246,11 @@ describe('useSubscription', () => { // But the intermediate ones should be ignored, // And the final rendered output should be the higher-priority observable. expect(Scheduler).toFlushAndYield([ - 'Child: b-0', - 'Subscriber: b-3', - 'Child: b-3', - 'Subscriber: a-0', - 'Child: a-0', + 'Inner: b-0', + 'Outer: b-3', + 'Inner: b-3', + 'Outer: a-0', + 'Inner: a-0', ]); expect(log).toEqual([ 'Parent.componentDidMount', @@ -263,32 +262,31 @@ describe('useSubscription', () => { it('should not drop values emitted between updates', () => { const log = []; - function Child({value}) { - Scheduler.yieldValue('Child: ' + value); - return null; + function Subscription({source}) { + const value = useSubscription( + () => ({ + getCurrentValue: () => source.getValue(), + subscribe: callback => { + const subscription = source.subscribe(callback); + return () => subscription.unsubscribe(); + }, + }), + [source], + ); + return ; } - const Subscription = createSubscription({ - getCurrentValue: source => source.getValue(), - subscribe: (source, callback) => { - const subscription = source.subscribe(callback); - return () => subscription.unsubscribe(); - }, - }); - - class Parent extends React.Component { - state = {}; - - static getDerivedStateFromProps(nextProps, prevState) { - if (nextProps.observed !== prevState.observed) { - return { - observed: nextProps.observed, - }; - } + function Outer({value}) { + Scheduler.yieldValue('Outer: ' + value); + return ; + } - return null; - } + function Inner({value}) { + Scheduler.yieldValue('Inner: ' + value); + return null; + } + class Parent extends React.Component { componentDidMount() { log.push('Parent.componentDidMount'); } @@ -298,14 +296,7 @@ describe('useSubscription', () => { } render() { - return ( - - {({value = 'default'}) => { - Scheduler.yieldValue('Subscriber: ' + value); - return ; - }} - - ); + return ; } } @@ -316,12 +307,12 @@ describe('useSubscription', () => { , {unstable_isConcurrent: true}, ); - expect(Scheduler).toFlushAndYield(['Subscriber: a-0', 'Child: a-0']); + expect(Scheduler).toFlushAndYield(['Outer: a-0', 'Inner: a-0']); expect(log).toEqual(['Parent.componentDidMount']); // Start React update, but don't finish renderer.update(); - expect(Scheduler).toFlushAndYieldThrough(['Subscriber: b-0']); + expect(Scheduler).toFlushAndYieldThrough(['Outer: b-0']); expect(log).toEqual(['Parent.componentDidMount']); // Emit some updates from the old subscribable @@ -335,9 +326,9 @@ describe('useSubscription', () => { // We expect the new subscribable to finish rendering, // But then the updated values from the old subscribable should be used. expect(Scheduler).toFlushAndYield([ - 'Child: b-0', - 'Subscriber: a-2', - 'Child: a-2', + 'Inner: b-0', + 'Outer: a-2', + 'Inner: a-2', ]); expect(log).toEqual([ 'Parent.componentDidMount', @@ -356,12 +347,24 @@ describe('useSubscription', () => { }); it('should guard against updates that happen after unmounting', () => { - const Subscription = createSubscription({ - getCurrentValue: source => source.getValue(), - subscribe: (source, callback) => { - return source.subscribe(callback); - }, - }); + function Subscription({source}) { + const value = useSubscription( + () => ({ + getCurrentValue: () => source.getValue(), + subscribe: callback => { + const unsubscribe = source.subscribe(callback); + return () => unsubscribe(); + }, + }), + [source], + ); + return ; + } + + function Child({value}) { + Scheduler.yieldValue(value); + return null; + } const eventHandler = { _callbacks: [], @@ -393,12 +396,7 @@ describe('useSubscription', () => { }); const renderer = ReactTestRenderer.create( - - {({value}) => { - Scheduler.yieldValue(value); - return null; - }} - , + , {unstable_isConcurrent: true}, ); diff --git a/packages/react-hooks/src/useSubscription.js b/packages/react-hooks/src/useSubscription.js index ee4078fe2c4c9..c749203cbaa81 100644 --- a/packages/react-hooks/src/useSubscription.js +++ b/packages/react-hooks/src/useSubscription.js @@ -1,30 +1,49 @@ -import {useEffect, useState} from 'react'; +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow + */ -export function useSubscription({ - // This is the thing being subscribed to (e.g. an observable, event dispatcher, etc). - source, +import {useEffect, useMemo, useState} from 'react'; - // (Synchronously) returns the current value of our subscription source. - getCurrentValue, +// Hook used for safely managing subscriptions in concurrent mode. +// It requires two parameters: a factory function and a dependencies array. +export function useSubscription( + // This function is called whenever the specified dependencies change. + // It should return an object with two keys (functions) documented below. + nextCreate: () => {| + // Get the current subscription value. + getCurrentValue: () => T, - // This function is passed an event handler to attach to the subscription source. - // It should return an unsubscribe function that removes the handler. - subscribe, -}) { - // Read the current value from our subscription source. + // This function is passed a callback to be called any time the subscription changes. + // It should return an unsubscribe function. + subscribe: (() => void) => () => void, + |}, + // Dependencies array. + // Any time one of the inputs change, a new subscription will be setup, + // and the previous listener will be unsubscribed. + deps: Array, +) { + const current = useMemo(nextCreate, deps); + + // Read the current subscription value. // When this value changes, we'll schedule an update with React. - // It's important to also store the source itself so that we can check for staleness. + // It's important to also store the current inputs as well so we can check for staleness. // (See the comment in checkForUpdates() below for more info.) const [state, setState] = useState({ - source, - value: getCurrentValue(source), + current, + value: current.getCurrentValue(), }); - // If the source has changed since our last render, schedule an update with its current value. - if (state.source !== source) { + // If the inputs have changed since our last render, schedule an update with the current value. + // We could do this in our effect handler below but there's no need to wait in this case. + if (state.current !== current) { setState({ - source, - value: getCurrentValue(source), + current, + value: current.getCurrentValue(), }); } @@ -51,18 +70,18 @@ export function useSubscription({ } setState(prevState => { - // Ignore values from stale sources! + // Ignore values from stale subscriptions! // Since we subscribe an unsubscribe in a passive effect, - // it's possible that this callback will be invoked for a stale (previous) source. - // This check avoids scheduling an update for htat stale source. - if (prevState.source !== source) { + // it's possible that this callback will be invoked for a stale (previous) subscription. + // This check avoids scheduling an update for the stale subscription. + if (prevState.current !== current) { return prevState; } - // Some subscription sources will auto-invoke the handler, even if the value hasn't changed. + // Some subscriptions will auto-invoke the handler when it's attached. // If the value hasn't changed, no update is needed. // Return state as-is so React can bail out and avoid an unnecessary render. - const value = getCurrentValue(source); + const value = current.getCurrentValue(); if (prevState.value === value) { return prevState; } @@ -70,7 +89,8 @@ export function useSubscription({ return {...prevState, value}; }); }; - const unsubscribe = subscribe(source, checkForUpdates); + + const unsubscribe = current.subscribe(checkForUpdates); // Because we're subscribing in a passive effect, // it's possible that an update has occurred between render and our effect handler. @@ -82,7 +102,7 @@ export function useSubscription({ unsubscribe(); }; }, - [getCurrentValue, source, subscribe], + [current], ); // Return the current value for our caller to use while rendering. From 3b3c15e1ac9bc54c8530c76327053b16d091eda5 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 7 Mar 2019 10:24:50 -0800 Subject: [PATCH 20/22] Revert "Updated useSubscription API to require deps array" This reverts commit 87f7c31c166a444900f616b617de73cb351881b4. --- .../useSubscription-test.internal.js | 266 +++++++++--------- packages/react-hooks/src/useSubscription.js | 72 ++--- 2 files changed, 160 insertions(+), 178 deletions(-) diff --git a/packages/react-hooks/src/__tests__/useSubscription-test.internal.js b/packages/react-hooks/src/__tests__/useSubscription-test.internal.js index 6f3ed4225d1ca..3027a6c4c4466 100644 --- a/packages/react-hooks/src/__tests__/useSubscription-test.internal.js +++ b/packages/react-hooks/src/__tests__/useSubscription-test.internal.js @@ -49,29 +49,34 @@ describe('useSubscription', () => { return replaySubject; } - it('supports basic subscription pattern', () => { - function Subscription({source}) { + // Mimic createSubscription API to simplify testing + function createSubscription({getCurrentValue, subscribe}) { + return function Subscription({children, source}) { const value = useSubscription( - () => ({ - getCurrentValue: () => source.getValue(), - subscribe: callback => { - const subscription = source.subscribe(callback); - return () => subscription.unsubscribe(); - }, - }), - [source], + React.useMemo(() => ({source, getCurrentValue, subscribe}), [source]), ); - return ; - } - function Child({value = 'default'}) { - Scheduler.yieldValue(value); - return null; - } + return React.createElement(children, {value}); + }; + } + + it('supports basic subscription pattern', () => { + const Subscription = createSubscription({ + getCurrentValue: source => source.getValue(), + subscribe: (source, callback) => { + const subscription = source.subscribe(callback); + return () => subscription.unsubscribe(); + }, + }); const observable = createBehaviorSubject(); const renderer = ReactTestRenderer.create( - , + + {({value = 'default'}) => { + Scheduler.yieldValue(value); + return null; + }} + , {unstable_isConcurrent: true}, ); @@ -89,36 +94,30 @@ describe('useSubscription', () => { }); it('should support observable types like RxJS ReplaySubject', () => { - function Subscription({source}) { - const value = useSubscription( - () => ({ - getCurrentValue: () => { - let currentValue; - source - .subscribe(tempValue => { - currentValue = tempValue; - }) - .unsubscribe(); - return currentValue; - }, - subscribe: callback => { - const subscription = source.subscribe(callback); - return () => subscription.unsubscribe(); - }, - }), - [source], - ); - return ; - } + const Subscription = createSubscription({ + getCurrentValue: source => { + let currentValue; + source + .subscribe(value => { + currentValue = value; + }) + .unsubscribe(); + return currentValue; + }, + subscribe: (source, callback) => { + const subscription = source.subscribe(callback); + return () => subscription.unsubscribe; + }, + }); - function Child({value = 'default'}) { + function render({value = 'default'}) { Scheduler.yieldValue(value); return null; } let observable = createReplaySubject('initial'); const renderer = ReactTestRenderer.create( - , + {render}, {unstable_isConcurrent: true}, ); expect(Scheduler).toFlushAndYield(['initial']); @@ -129,26 +128,20 @@ describe('useSubscription', () => { // Unsetting the subscriber prop should reset subscribed values observable = createReplaySubject(undefined); - renderer.update(); + renderer.update({render}); expect(Scheduler).toFlushAndYield(['default']); }); it('should unsubscribe from old subscribables and subscribe to new subscribables when props change', () => { - function Subscription({source}) { - const value = useSubscription( - () => ({ - getCurrentValue: () => source.getValue(), - subscribe: callback => { - const subscription = source.subscribe(callback); - return () => subscription.unsubscribe(); - }, - }), - [source], - ); - return ; - } + const Subscription = createSubscription({ + getCurrentValue: source => source.getValue(), + subscribe: (source, callback) => { + const subscription = source.subscribe(callback); + return () => subscription.unsubscribe(); + }, + }); - function Child({value = 'default'}) { + function render({value = 'default'}) { Scheduler.yieldValue(value); return null; } @@ -157,7 +150,7 @@ describe('useSubscription', () => { const observableB = createBehaviorSubject('b-0'); const renderer = ReactTestRenderer.create( - , + {render}, {unstable_isConcurrent: true}, ); @@ -165,7 +158,7 @@ describe('useSubscription', () => { expect(Scheduler).toFlushAndYield(['a-0']); // Unsetting the subscriber prop should reset subscribed values - renderer.update(); + renderer.update({render}); expect(Scheduler).toFlushAndYield(['b-0']); // Updates to the old subscribable should not re-render the child component @@ -180,31 +173,32 @@ describe('useSubscription', () => { it('should ignore values emitted by a new subscribable until the commit phase', () => { const log = []; - function Subscription({source}) { - const value = useSubscription( - () => ({ - getCurrentValue: () => source.getValue(), - subscribe: callback => { - const subscription = source.subscribe(callback); - return () => subscription.unsubscribe(); - }, - }), - [source], - ); - return ; - } - - function Outer({value}) { - Scheduler.yieldValue('Outer: ' + value); - return ; - } - - function Inner({value}) { - Scheduler.yieldValue('Inner: ' + value); + function Child({value}) { + Scheduler.yieldValue('Child: ' + value); return null; } + const Subscription = createSubscription({ + getCurrentValue: source => source.getValue(), + subscribe: (source, callback) => { + const subscription = source.subscribe(callback); + return () => subscription.unsubscribe(); + }, + }); + class Parent extends React.Component { + state = {}; + + static getDerivedStateFromProps(nextProps, prevState) { + if (nextProps.observed !== prevState.observed) { + return { + observed: nextProps.observed, + }; + } + + return null; + } + componentDidMount() { log.push('Parent.componentDidMount'); } @@ -214,7 +208,14 @@ describe('useSubscription', () => { } render() { - return ; + return ( + + {({value = 'default'}) => { + Scheduler.yieldValue('Subscriber: ' + value); + return ; + }} + + ); } } @@ -225,12 +226,12 @@ describe('useSubscription', () => { , {unstable_isConcurrent: true}, ); - expect(Scheduler).toFlushAndYield(['Outer: a-0', 'Inner: a-0']); + expect(Scheduler).toFlushAndYield(['Subscriber: a-0', 'Child: a-0']); expect(log).toEqual(['Parent.componentDidMount']); // Start React update, but don't finish renderer.update(); - expect(Scheduler).toFlushAndYieldThrough(['Outer: b-0']); + expect(Scheduler).toFlushAndYieldThrough(['Subscriber: b-0']); expect(log).toEqual(['Parent.componentDidMount']); // Emit some updates from the uncommitted subscribable @@ -246,11 +247,11 @@ describe('useSubscription', () => { // But the intermediate ones should be ignored, // And the final rendered output should be the higher-priority observable. expect(Scheduler).toFlushAndYield([ - 'Inner: b-0', - 'Outer: b-3', - 'Inner: b-3', - 'Outer: a-0', - 'Inner: a-0', + 'Child: b-0', + 'Subscriber: b-3', + 'Child: b-3', + 'Subscriber: a-0', + 'Child: a-0', ]); expect(log).toEqual([ 'Parent.componentDidMount', @@ -262,31 +263,32 @@ describe('useSubscription', () => { it('should not drop values emitted between updates', () => { const log = []; - function Subscription({source}) { - const value = useSubscription( - () => ({ - getCurrentValue: () => source.getValue(), - subscribe: callback => { - const subscription = source.subscribe(callback); - return () => subscription.unsubscribe(); - }, - }), - [source], - ); - return ; - } - - function Outer({value}) { - Scheduler.yieldValue('Outer: ' + value); - return ; - } - - function Inner({value}) { - Scheduler.yieldValue('Inner: ' + value); + function Child({value}) { + Scheduler.yieldValue('Child: ' + value); return null; } + const Subscription = createSubscription({ + getCurrentValue: source => source.getValue(), + subscribe: (source, callback) => { + const subscription = source.subscribe(callback); + return () => subscription.unsubscribe(); + }, + }); + class Parent extends React.Component { + state = {}; + + static getDerivedStateFromProps(nextProps, prevState) { + if (nextProps.observed !== prevState.observed) { + return { + observed: nextProps.observed, + }; + } + + return null; + } + componentDidMount() { log.push('Parent.componentDidMount'); } @@ -296,7 +298,14 @@ describe('useSubscription', () => { } render() { - return ; + return ( + + {({value = 'default'}) => { + Scheduler.yieldValue('Subscriber: ' + value); + return ; + }} + + ); } } @@ -307,12 +316,12 @@ describe('useSubscription', () => { , {unstable_isConcurrent: true}, ); - expect(Scheduler).toFlushAndYield(['Outer: a-0', 'Inner: a-0']); + expect(Scheduler).toFlushAndYield(['Subscriber: a-0', 'Child: a-0']); expect(log).toEqual(['Parent.componentDidMount']); // Start React update, but don't finish renderer.update(); - expect(Scheduler).toFlushAndYieldThrough(['Outer: b-0']); + expect(Scheduler).toFlushAndYieldThrough(['Subscriber: b-0']); expect(log).toEqual(['Parent.componentDidMount']); // Emit some updates from the old subscribable @@ -326,9 +335,9 @@ describe('useSubscription', () => { // We expect the new subscribable to finish rendering, // But then the updated values from the old subscribable should be used. expect(Scheduler).toFlushAndYield([ - 'Inner: b-0', - 'Outer: a-2', - 'Inner: a-2', + 'Child: b-0', + 'Subscriber: a-2', + 'Child: a-2', ]); expect(log).toEqual([ 'Parent.componentDidMount', @@ -347,24 +356,12 @@ describe('useSubscription', () => { }); it('should guard against updates that happen after unmounting', () => { - function Subscription({source}) { - const value = useSubscription( - () => ({ - getCurrentValue: () => source.getValue(), - subscribe: callback => { - const unsubscribe = source.subscribe(callback); - return () => unsubscribe(); - }, - }), - [source], - ); - return ; - } - - function Child({value}) { - Scheduler.yieldValue(value); - return null; - } + const Subscription = createSubscription({ + getCurrentValue: source => source.getValue(), + subscribe: (source, callback) => { + return source.subscribe(callback); + }, + }); const eventHandler = { _callbacks: [], @@ -396,7 +393,12 @@ describe('useSubscription', () => { }); const renderer = ReactTestRenderer.create( - , + + {({value}) => { + Scheduler.yieldValue(value); + return null; + }} + , {unstable_isConcurrent: true}, ); diff --git a/packages/react-hooks/src/useSubscription.js b/packages/react-hooks/src/useSubscription.js index c749203cbaa81..ee4078fe2c4c9 100644 --- a/packages/react-hooks/src/useSubscription.js +++ b/packages/react-hooks/src/useSubscription.js @@ -1,49 +1,30 @@ -/** - * Copyright (c) Facebook, Inc. and its affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - * - * @flow - */ +import {useEffect, useState} from 'react'; -import {useEffect, useMemo, useState} from 'react'; +export function useSubscription({ + // This is the thing being subscribed to (e.g. an observable, event dispatcher, etc). + source, -// Hook used for safely managing subscriptions in concurrent mode. -// It requires two parameters: a factory function and a dependencies array. -export function useSubscription( - // This function is called whenever the specified dependencies change. - // It should return an object with two keys (functions) documented below. - nextCreate: () => {| - // Get the current subscription value. - getCurrentValue: () => T, + // (Synchronously) returns the current value of our subscription source. + getCurrentValue, - // This function is passed a callback to be called any time the subscription changes. - // It should return an unsubscribe function. - subscribe: (() => void) => () => void, - |}, - // Dependencies array. - // Any time one of the inputs change, a new subscription will be setup, - // and the previous listener will be unsubscribed. - deps: Array, -) { - const current = useMemo(nextCreate, deps); - - // Read the current subscription value. + // This function is passed an event handler to attach to the subscription source. + // It should return an unsubscribe function that removes the handler. + subscribe, +}) { + // Read the current value from our subscription source. // When this value changes, we'll schedule an update with React. - // It's important to also store the current inputs as well so we can check for staleness. + // It's important to also store the source itself so that we can check for staleness. // (See the comment in checkForUpdates() below for more info.) const [state, setState] = useState({ - current, - value: current.getCurrentValue(), + source, + value: getCurrentValue(source), }); - // If the inputs have changed since our last render, schedule an update with the current value. - // We could do this in our effect handler below but there's no need to wait in this case. - if (state.current !== current) { + // If the source has changed since our last render, schedule an update with its current value. + if (state.source !== source) { setState({ - current, - value: current.getCurrentValue(), + source, + value: getCurrentValue(source), }); } @@ -70,18 +51,18 @@ export function useSubscription( } setState(prevState => { - // Ignore values from stale subscriptions! + // Ignore values from stale sources! // Since we subscribe an unsubscribe in a passive effect, - // it's possible that this callback will be invoked for a stale (previous) subscription. - // This check avoids scheduling an update for the stale subscription. - if (prevState.current !== current) { + // it's possible that this callback will be invoked for a stale (previous) source. + // This check avoids scheduling an update for htat stale source. + if (prevState.source !== source) { return prevState; } - // Some subscriptions will auto-invoke the handler when it's attached. + // Some subscription sources will auto-invoke the handler, even if the value hasn't changed. // If the value hasn't changed, no update is needed. // Return state as-is so React can bail out and avoid an unnecessary render. - const value = current.getCurrentValue(); + const value = getCurrentValue(source); if (prevState.value === value) { return prevState; } @@ -89,8 +70,7 @@ export function useSubscription( return {...prevState, value}; }); }; - - const unsubscribe = current.subscribe(checkForUpdates); + const unsubscribe = subscribe(source, checkForUpdates); // Because we're subscribing in a passive effect, // it's possible that an update has occurred between render and our effect handler. @@ -102,7 +82,7 @@ export function useSubscription( unsubscribe(); }; }, - [current], + [getCurrentValue, source, subscribe], ); // Return the current value for our caller to use while rendering. From a4ce2f4e05be92db6d82ebfd8ea6ba2751933e1d Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 7 Mar 2019 10:38:41 -0800 Subject: [PATCH 21/22] Tidied things up, added Flow types, etc --- .../useSubscription-test.internal.js | 33 +++++++------------ packages/react-hooks/src/useSubscription.js | 23 +++++++++++-- 2 files changed, 32 insertions(+), 24 deletions(-) diff --git a/packages/react-hooks/src/__tests__/useSubscription-test.internal.js b/packages/react-hooks/src/__tests__/useSubscription-test.internal.js index 3027a6c4c4466..fe9eaafe26077 100644 --- a/packages/react-hooks/src/__tests__/useSubscription-test.internal.js +++ b/packages/react-hooks/src/__tests__/useSubscription-test.internal.js @@ -51,12 +51,16 @@ describe('useSubscription', () => { // Mimic createSubscription API to simplify testing function createSubscription({getCurrentValue, subscribe}) { + function DefaultChild({value = 'default'}) { + Scheduler.yieldValue(value); + return null; + } + return function Subscription({children, source}) { const value = useSubscription( React.useMemo(() => ({source, getCurrentValue, subscribe}), [source]), ); - - return React.createElement(children, {value}); + return React.createElement(children || DefaultChild, {value}); }; } @@ -71,12 +75,7 @@ describe('useSubscription', () => { const observable = createBehaviorSubject(); const renderer = ReactTestRenderer.create( - - {({value = 'default'}) => { - Scheduler.yieldValue(value); - return null; - }} - , + , {unstable_isConcurrent: true}, ); @@ -110,14 +109,9 @@ describe('useSubscription', () => { }, }); - function render({value = 'default'}) { - Scheduler.yieldValue(value); - return null; - } - let observable = createReplaySubject('initial'); const renderer = ReactTestRenderer.create( - {render}, + , {unstable_isConcurrent: true}, ); expect(Scheduler).toFlushAndYield(['initial']); @@ -128,7 +122,7 @@ describe('useSubscription', () => { // Unsetting the subscriber prop should reset subscribed values observable = createReplaySubject(undefined); - renderer.update({render}); + renderer.update(); expect(Scheduler).toFlushAndYield(['default']); }); @@ -141,16 +135,11 @@ describe('useSubscription', () => { }, }); - function render({value = 'default'}) { - Scheduler.yieldValue(value); - return null; - } - const observableA = createBehaviorSubject('a-0'); const observableB = createBehaviorSubject('b-0'); const renderer = ReactTestRenderer.create( - {render}, + , {unstable_isConcurrent: true}, ); @@ -158,7 +147,7 @@ describe('useSubscription', () => { expect(Scheduler).toFlushAndYield(['a-0']); // Unsetting the subscriber prop should reset subscribed values - renderer.update({render}); + renderer.update(); expect(Scheduler).toFlushAndYield(['b-0']); // Updates to the old subscribable should not re-render the child component diff --git a/packages/react-hooks/src/useSubscription.js b/packages/react-hooks/src/useSubscription.js index ee4078fe2c4c9..0a5a2da8a6a0a 100644 --- a/packages/react-hooks/src/useSubscription.js +++ b/packages/react-hooks/src/useSubscription.js @@ -1,6 +1,21 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow + */ + import {useEffect, useState} from 'react'; -export function useSubscription({ +// Hook used for safely managing subscriptions in concurrent mode. +// +// In order to avoid removing and re-adding subscriptions each time this hook is called, +// the parameters passed to this hook should be memoized in some way– +// either by wrapping the entire params object with useMemo() +// or by wrapping the individual callbacks with useCallback(). +export function useSubscription({ // This is the thing being subscribed to (e.g. an observable, event dispatcher, etc). source, @@ -10,7 +25,11 @@ export function useSubscription({ // This function is passed an event handler to attach to the subscription source. // It should return an unsubscribe function that removes the handler. subscribe, -}) { +}: {| + source: Source, + getCurrentValue: (source: Source) => Value, + subscribe: (source: Source, callback: Function) => () => void, +|}) { // Read the current value from our subscription source. // When this value changes, we'll schedule an update with React. // It's important to also store the source itself so that we can check for staleness. From 140cdb806ef732417fee536626e9318ed2bd83f4 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 11 Mar 2019 14:51:23 -0700 Subject: [PATCH 22/22] Add return type to useSubscription --- packages/react-hooks/src/useSubscription.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-hooks/src/useSubscription.js b/packages/react-hooks/src/useSubscription.js index 0a5a2da8a6a0a..8a564c8a180e9 100644 --- a/packages/react-hooks/src/useSubscription.js +++ b/packages/react-hooks/src/useSubscription.js @@ -29,7 +29,7 @@ export function useSubscription({ source: Source, getCurrentValue: (source: Source) => Value, subscribe: (source: Source, callback: Function) => () => void, -|}) { +|}): Value { // Read the current value from our subscription source. // When this value changes, we'll schedule an update with React. // It's important to also store the source itself so that we can check for staleness.