-
Notifications
You must be signed in to change notification settings - Fork 47.4k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Update on "compiler: Allow global mutation in jsx props"
Fixes https://x.com/raibima/status/1794395807216738792 The issue is that if you pass a global-modifying function as prop to JSX, we currently report that it's invalid to modify a global during rendering. The problem is that we don't really know when/if the child component will actually call that function prop. It would be against the rules to call the function during render, but it's totally fine to call it during an event handler or from a useEffect. Since we don't know at the call-site how the child will use the function, we should allow such calls. In the future we could improve this in a few ways: * For all functions that modify globals, codegen an assertion or warning into the function that fires if it's called "during render". We'd have to precisely define what "during render" is, but this would at least help developers catch this dynamically. * Use the type system to distinguish "event/effect" and "render" functions to help developers avoid accidentally mutating globals during render. [ghstack-poisoned]
- Loading branch information
1 parent
2e0e571
commit a15a43e
Showing
6 changed files
with
105 additions
and
0 deletions.
There are no files selected for viewing
27 changes: 27 additions & 0 deletions
27
...sts__/fixtures/compiler/error.assign-global-in-component-tag-function.expect.md
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
|
||
## Input | ||
|
||
```javascript | ||
function Component() { | ||
const Foo = () => { | ||
someGlobal = true; | ||
}; | ||
return <Foo />; | ||
} | ||
|
||
``` | ||
|
||
|
||
## Error | ||
|
||
``` | ||
1 | function Component() { | ||
2 | const Foo = () => { | ||
> 3 | someGlobal = true; | ||
| ^^^^^^^^^^ InvalidReact: Unexpected reassignment of a variable which was defined outside of the component. Components and hooks should be pure and side-effect free, but variable reassignment is a form of side-effect. If this variable is used in rendering, use useState instead. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render) (3:3) | ||
4 | }; | ||
5 | return <Foo />; | ||
6 | } | ||
``` | ||
6 changes: 6 additions & 0 deletions
6
...compiler/src/__tests__/fixtures/compiler/error.assign-global-in-component-tag-function.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
function Component() { | ||
const Foo = () => { | ||
someGlobal = true; | ||
}; | ||
return <Foo />; | ||
} |
30 changes: 30 additions & 0 deletions
30
...r/src/__tests__/fixtures/compiler/error.assign-global-in-jsx-children.expect.md
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
|
||
## Input | ||
|
||
```javascript | ||
function Component() { | ||
const foo = () => { | ||
someGlobal = true; | ||
}; | ||
// Children are generally access/called during render, so | ||
// modifying a global in a children function is almost | ||
// certainly a mistake. | ||
return <Foo>{foo}</Foo>; | ||
} | ||
|
||
``` | ||
|
||
|
||
## Error | ||
|
||
``` | ||
1 | function Component() { | ||
2 | const foo = () => { | ||
> 3 | someGlobal = true; | ||
| ^^^^^^^^^^ InvalidReact: Unexpected reassignment of a variable which was defined outside of the component. Components and hooks should be pure and side-effect free, but variable reassignment is a form of side-effect. If this variable is used in rendering, use useState instead. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render) (3:3) | ||
4 | }; | ||
5 | // Children are generally access/called during render, so | ||
6 | // modifying a global in a children function is almost | ||
``` | ||
9 changes: 9 additions & 0 deletions
9
...gin-react-compiler/src/__tests__/fixtures/compiler/error.assign-global-in-jsx-children.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
function Component() { | ||
const foo = () => { | ||
someGlobal = true; | ||
}; | ||
// Children are generally access/called during render, so | ||
// modifying a global in a children function is almost | ||
// certainly a mistake. | ||
return <Foo>{foo}</Foo>; | ||
} |
27 changes: 27 additions & 0 deletions
27
...tests__/fixtures/compiler/error.assign-global-in-jsx-spread-attribute.expect.md
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
|
||
## Input | ||
|
||
```javascript | ||
function Component() { | ||
const foo = () => { | ||
someGlobal = true; | ||
}; | ||
return <div {...foo} />; | ||
} | ||
|
||
``` | ||
|
||
|
||
## Error | ||
|
||
``` | ||
1 | function Component() { | ||
2 | const foo = () => { | ||
> 3 | someGlobal = true; | ||
| ^^^^^^^^^^ InvalidReact: Unexpected reassignment of a variable which was defined outside of the component. Components and hooks should be pure and side-effect free, but variable reassignment is a form of side-effect. If this variable is used in rendering, use useState instead. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render) (3:3) | ||
4 | }; | ||
5 | return <div {...foo} />; | ||
6 | } | ||
``` | ||
6 changes: 6 additions & 0 deletions
6
...t-compiler/src/__tests__/fixtures/compiler/error.assign-global-in-jsx-spread-attribute.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
function Component() { | ||
const foo = () => { | ||
someGlobal = true; | ||
}; | ||
return <div {...foo} />; | ||
} |