-
Notifications
You must be signed in to change notification settings - Fork 49.9k
[compiler] Validate against component/hook factories #34305
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -494,7 +494,20 @@ function findFunctionsToCompile( | |
| ): Array<CompileSource> { | ||
| const queue: Array<CompileSource> = []; | ||
| const traverseFunction = (fn: BabelFn, pass: CompilerPass): void => { | ||
| // In 'all' mode, compile only top level functions | ||
| if ( | ||
| pass.opts.compilationMode === 'all' && | ||
| fn.scope.getProgramParent() !== fn.scope.parent | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| const fnType = getReactFunctionType(fn, pass); | ||
|
|
||
| if (pass.opts.environment.validateNoDynamicallyCreatedComponentsOrHooks) { | ||
| validateNoDynamicallyCreatedComponentsOrHooks(fn, pass, programContext); | ||
| } | ||
|
|
||
| if (fnType === null || programContext.alreadyCompiled.has(fn.node)) { | ||
| return; | ||
| } | ||
|
|
@@ -839,6 +852,73 @@ function shouldSkipCompilation( | |
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * Validates that Components/Hooks are always defined at module level. This prevents scope reference | ||
| * errors that occur when the compiler attempts to optimize the nested component/hook while its | ||
| * parent function remains uncompiled. | ||
| */ | ||
| function validateNoDynamicallyCreatedComponentsOrHooks( | ||
| fn: BabelFn, | ||
| pass: CompilerPass, | ||
| programContext: ProgramContext, | ||
| ): void { | ||
| const parentNameExpr = getFunctionName(fn); | ||
| const parentName = | ||
| parentNameExpr !== null && parentNameExpr.isIdentifier() | ||
| ? parentNameExpr.node.name | ||
| : '<anonymous>'; | ||
|
|
||
| const validateNestedFunction = ( | ||
| nestedFn: NodePath< | ||
| t.FunctionDeclaration | t.FunctionExpression | t.ArrowFunctionExpression | ||
| >, | ||
| ): void => { | ||
| if ( | ||
| nestedFn.node === fn.node || | ||
| programContext.alreadyCompiled.has(nestedFn.node) | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| if (nestedFn.scope.getProgramParent() !== nestedFn.scope.parent) { | ||
| const nestedFnType = getReactFunctionType(nestedFn as BabelFn, pass); | ||
| const nestedFnNameExpr = getFunctionName(nestedFn as BabelFn); | ||
| const nestedName = | ||
| nestedFnNameExpr !== null && nestedFnNameExpr.isIdentifier() | ||
| ? nestedFnNameExpr.node.name | ||
| : '<anonymous>'; | ||
| if (nestedFnType === 'Component' || nestedFnType === 'Hook') { | ||
| CompilerError.throwDiagnostic({ | ||
| category: ErrorCategory.Factories, | ||
| severity: ErrorSeverity.InvalidReact, | ||
| reason: `Components and hooks cannot be created dynamically`, | ||
| description: `The function \`${nestedName}\` appears to be a React ${nestedFnType.toLowerCase()}, but it's defined inside \`${parentName}\`. Components and Hooks should always be declared at module scope`, | ||
| details: [ | ||
| { | ||
| kind: 'error', | ||
| message: 'this function dynamically created a component/hook', | ||
| loc: parentNameExpr?.node.loc ?? fn.node.loc ?? null, | ||
| }, | ||
| { | ||
| kind: 'error', | ||
| message: 'the component is created here', | ||
| loc: nestedFnNameExpr?.node.loc ?? nestedFn.node.loc ?? null, | ||
| }, | ||
| ], | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| nestedFn.skip(); | ||
| }; | ||
|
|
||
| fn.traverse({ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I understand correctly
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah this would traverse any nested Functions and just check each parent/child pair to see if the parent and child are both React functions |
||
| FunctionDeclaration: validateNestedFunction, | ||
| FunctionExpression: validateNestedFunction, | ||
| ArrowFunctionExpression: validateNestedFunction, | ||
| }); | ||
| } | ||
|
|
||
| function getReactFunctionType( | ||
| fn: BabelFn, | ||
| pass: CompilerPass, | ||
|
|
@@ -877,11 +957,6 @@ function getReactFunctionType( | |
| return componentSyntaxType; | ||
| } | ||
| case 'all': { | ||
| // Compile only top level functions | ||
| if (fn.scope.getProgramParent() !== fn.scope.parent) { | ||
| return null; | ||
| } | ||
|
|
||
| return getComponentOrHookLike(fn, hookPattern) ?? 'Other'; | ||
| } | ||
| default: { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
|
|
||
| ## Input | ||
|
|
||
| ```javascript | ||
| // @validateNoDynamicallyCreatedComponentsOrHooks | ||
| export function getInput(a) { | ||
| const Wrapper = () => { | ||
| const handleChange = () => { | ||
| a.onChange(); | ||
| }; | ||
|
|
||
| return <input onChange={handleChange} />; | ||
| }; | ||
|
|
||
| return Wrapper; | ||
| } | ||
|
|
||
| export const FIXTURE_ENTRYPOINT = { | ||
| fn: getInput, | ||
| isComponent: false, | ||
| params: [{onChange() {}}], | ||
| }; | ||
|
|
||
| ``` | ||
|
|
||
|
|
||
| ## Error | ||
|
|
||
| ``` | ||
| Found 1 error: | ||
|
|
||
| Error: Components and hooks cannot be created dynamically | ||
|
|
||
| The function `Wrapper` appears to be a React component, but it's defined inside `getInput`. Components and Hooks should always be declared at module scope | ||
|
|
||
| error.nested-component-in-normal-function.ts:2:16 | ||
| 1 | // @validateNoDynamicallyCreatedComponentsOrHooks | ||
| > 2 | export function getInput(a) { | ||
| | ^^^^^^^^ this function dynamically created a component/hook | ||
| 3 | const Wrapper = () => { | ||
| 4 | const handleChange = () => { | ||
| 5 | a.onChange(); | ||
|
|
||
| error.nested-component-in-normal-function.ts:3:8 | ||
| 1 | // @validateNoDynamicallyCreatedComponentsOrHooks | ||
| 2 | export function getInput(a) { | ||
| > 3 | const Wrapper = () => { | ||
| | ^^^^^^^ the component is created here | ||
| 4 | const handleChange = () => { | ||
| 5 | a.onChange(); | ||
| 6 | }; | ||
| ``` | ||
|
|
||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| // @validateNoDynamicallyCreatedComponentsOrHooks | ||
| export function getInput(a) { | ||
| const Wrapper = () => { | ||
| const handleChange = () => { | ||
| a.onChange(); | ||
| }; | ||
|
|
||
| return <input onChange={handleChange} />; | ||
| }; | ||
|
|
||
| return Wrapper; | ||
| } | ||
|
|
||
| export const FIXTURE_ENTRYPOINT = { | ||
| fn: getInput, | ||
| isComponent: false, | ||
| params: [{onChange() {}}], | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
|
|
||
| ## Input | ||
|
|
||
| ```javascript | ||
| // @validateNoDynamicallyCreatedComponentsOrHooks | ||
| import {useState} from 'react'; | ||
|
|
||
| function createCustomHook(config) { | ||
| function useConfiguredState() { | ||
| const [state, setState] = useState(0); | ||
|
|
||
| const increment = () => { | ||
| setState(state + config.step); | ||
| }; | ||
|
|
||
| return [state, increment]; | ||
| } | ||
|
|
||
| return useConfiguredState; | ||
| } | ||
|
|
||
| export const FIXTURE_ENTRYPOINT = { | ||
| fn: createCustomHook, | ||
| isComponent: false, | ||
| params: [{step: 1}], | ||
| }; | ||
|
|
||
| ``` | ||
|
|
||
|
|
||
| ## Error | ||
|
|
||
| ``` | ||
| Found 1 error: | ||
|
|
||
| Error: Components and hooks cannot be created dynamically | ||
|
|
||
| The function `useConfiguredState` appears to be a React hook, but it's defined inside `createCustomHook`. Components and Hooks should always be declared at module scope | ||
|
|
||
| error.nested-hook-in-normal-function.ts:4:9 | ||
| 2 | import {useState} from 'react'; | ||
| 3 | | ||
| > 4 | function createCustomHook(config) { | ||
| | ^^^^^^^^^^^^^^^^ this function dynamically created a component/hook | ||
| 5 | function useConfiguredState() { | ||
| 6 | const [state, setState] = useState(0); | ||
| 7 | | ||
|
|
||
| error.nested-hook-in-normal-function.ts:5:11 | ||
| 3 | | ||
| 4 | function createCustomHook(config) { | ||
| > 5 | function useConfiguredState() { | ||
| | ^^^^^^^^^^^^^^^^^^ the component is created here | ||
| 6 | const [state, setState] = useState(0); | ||
| 7 | | ||
| 8 | const increment = () => { | ||
| ``` | ||
|
|
||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| // @validateNoDynamicallyCreatedComponentsOrHooks | ||
| import {useState} from 'react'; | ||
|
|
||
| function createCustomHook(config) { | ||
| function useConfiguredState() { | ||
| const [state, setState] = useState(0); | ||
|
|
||
| const increment = () => { | ||
| setState(state + config.step); | ||
| }; | ||
|
|
||
| return [state, increment]; | ||
| } | ||
|
|
||
| return useConfiguredState; | ||
| } | ||
|
|
||
| export const FIXTURE_ENTRYPOINT = { | ||
| fn: createCustomHook, | ||
| isComponent: false, | ||
| params: [{step: 1}], | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would
fn.scope.getProgramParent === fn.scope.parentmean vs. what you wrote in this line?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scope.getProgramParent()is a Babel API that returns theProgramnode. Comparing these 2 nodes just checks if thefnis in module scope (if it was in module scope, it's parent would also be the Program), or if it's nested within another scope (such as in a BlockStatement). These ASTexplorer console logs might help visualize that better