Reusing DEFAULT_HOOKS instance is a bit scary in case we mutate it by mistake
and this gets reflected across all compiles.
This isn't a concern now as we can't change the config during compilation. But
this PR keeps us safe in case we change this behavior in the future.
The tradeoff is a bit of perf which I think is the right tradeoff here.
I did a double take when I thought we didn't handle returning the
error when reading the code and when I edited the code, typescript told
me that there's no need to return as creating the error will throw.
This PR makes it clear from the name of the function that we will throw.
We should only add imports if we actually compiled anything, this is what caused
the internal issue despite the file in question not having any functions
opted-in to compilation.
When an `environment` isn't explicitly provided by the user's config, we used to
default this to `null` in `parsePluginOptions` which is called right at the
start of the Babel plugin. These parsed options were then being passed to zod,
which was expecting an object type, not null.
Zod can reify a default config based on the schema, if an empty object is passed
in. So by changing our default value to `{}` this should fix the compiler
bailing out incorrectly on valid compiler options
Test plan: tested this manually on the external repo by modifying node_modules
Use zod to do runtime validation and throw if incorrect.
This PR only adds validation for ExternalFunction, will validate other options
in follow on PRs.
This PR adds a feature flag to model a potential new-in-practice rule in React:
that freezing a function expression also freezes its closed-over values,
transitively. For example, in the following code `data` is frozen when the
lambda that captures it is is passed to useEffect:
```javascript
const data = [];
// useEffect freezes its argument (the function expr), which transitively
freezes its captured value data
useEffect(() => {
foo(data);
}, [data]);
data.push(true); // ERROR: mutating a frozen value
mutate(data); // we conservatively assume this doesn't mutate but could be wrong
```
Note that this rule has never been written down or enforced. It is theoretically
equivalent to the rule (already implemented in Forget) that values captured by
JSX are frozen:
```javascript
const style = {...};
<div style={style}>...</div>
style.width = 10; // ERROR: mutating a frozen value
mutate(style); // we conservatively assume this doesn't mutate but could be
wrong
```
However, JSX is typically constructed toward the very end of a render function.
Thus in practice there isn't much subsequent code that could even modify such a
captured value. But for the useEffect case (and other hooks that take closures
as arguments), they tend to occur much earlier in a render function. There's
more code that can run later and still modify the captured values, without
causing issues in practice. The _practical_ rule today is that you can't modify
values captured by frozen lambdas _after the component returns_: it's fine in
practice to modify captured values between calling eg useEffect and returning
from render.
Thus this feature flag is fairly likely to break some percent of real product
code. I'm adding this so that we can experiment and see how unsafe it actually
is.
Adds an internal compiler assertion pass which checks that all the instructions
which are necessary for constructing a given scope correctly end up within the
corresponding ReactiveScopeBlock. All known cases where this can occur are fixed
earlier in the stack, but this assertion will help us catch any other cases we
haven't thought of. See docblock comment for more info.
## Test Plan
I manually reverted the fixes from the previous PRs while keeping the new
fixtures, and verified that this new assertion pass flags the fixtures as
invalid.
The previous changes mostly meant that we removed the label terminal and didn't
have instructions for the same scope split in a way that we couldn't merge. But
logicals were still causing a split because MergeConsecutiveScopes can't merge
the blocks in that case. Here we move PruneUnusedLabels earlier in the pipeline
to ensure that instructions from IIFEs have floated up to the parent block scope
level.
Fixes for the previous PR. What was happening is that our inference was
inferring the correct mutable ranges and reactive scopes, but the inlining
process left the instructions from the IIFEs inside a separate block, with a
'label' terminal preceding it. When we converted to ReactiveFunction this was
preserved as a ReactiveLabelTerminal, which meant that the first instruction for
the mutable range could be nested inside one LabelTerminal, while more would be
in a subsequent LabelTerminal. But we close blocks based on the block scope!
This meant that we'd have leftover instructions (in the second LabelTerminal)
that got left out of the block.
Furthermore, because inlining was happening after EnterSSA we weren't creating
phis correctly. This PR fixes a bunch of these issues, and a subsequent PR
handles the remaining cases:
* We move DropManualMemo and InlineIIFEs before EnterSSA. This means we lose the
ability to use type information, but we ensure that we create proper SSA ids and
phis for any reassignments within the IIFE
* We also update PruneUnusedLabels to not just remove the unused labels, but to
actually remove LabelTerminals that don't need them.
We construct invalid mutable ranges in these cases because the range starts
within a labeled block. We need to run merge consecutive scopes and EnterSSA
after inlining so that the code is lifted out of the labeled block to the
correct scope, and so that we create phis for reassignments within the IIFE.
This has caused issues for people when things like Babel cause issues. It's not
actionable and it crashes eslint. Just like the Babel plugin, the eslint plugin
should never throw. Instead, let's log the error so the data isn't lost.
This combines our scripts and makes it so we no longer need to create a separate
commit to add the forget-feedback/dist directory into the react-forget repo. I
originally did that so I could run tests here, but now that the external repo is
created and has its test suite hooked up to CI, this is now unnecessary friction
to run a sync
All of these warnings go away: ``` ➜ playground git:(remove-prettier) ✗ yarn
dev yarn run v1.22.19 $ NODE_ENV=development && next dev ready - started server
on 0.0.0.0:3000, url: http://localhost:3000 warn - You have enabled
experimental feature (appDir) in next.config.js. warn - Experimental features
are not covered by semver, and may cause unexpected or broken application
behavior. Use at your own risk. info - Thank you for testing `appDir` please
leave your feedback at https://nextjs.link/app-feedback
event - compiled client and server successfully in 964 ms (199 modules) wait -
compiling /page (client and server)... warn -
../../node_modules/prettier/index.js Critical dependency: the request of a
dependency is an expression
../../node_modules/prettier/index.js Critical dependency: require function is
used in a way in which dependencies cannot be statically extracted
../../node_modules/prettier/index.js Critical dependency: the request of a
dependency is an expression
../../node_modules/prettier/index.js Critical dependency: the request of a
dependency is an expression
../../node_modules/prettier/third-party.js Critical dependency: the request of a
dependency is an expression wait - compiling... warn -
../../node_modules/prettier/index.js Critical dependency: the request of a
dependency is an expression
../../node_modules/prettier/index.js Critical dependency: require function is
used in a way in which dependencies cannot be statically extracted
../../node_modules/prettier/index.js Critical dependency: the request of a
dependency is an expression
../../node_modules/prettier/index.js Critical dependency: the request of a
dependency is an expression
../../node_modules/prettier/third-party.js Critical dependency: the request of a
dependency is an expression ```
Standalone prettier is meant to be used in the browser:
https://prettier.io/docs/en/browser
I don't know if it's possible to write a test for this as I can't seem to get
the codegen to change.
For the following testcase: ``` function useFoo(setOne) { let x; let y; if
(setOne) { x = 1; y = 3; } else { x = 2; y = 5; }
return { x, y }; } ```
The LeaveSSA changes from: ``` .... bb1 (block): predecessor blocks: bb2 bb3
x$36:TPrimitive: phi(bb2: x$19, bb3: x$19) y$21[8:14]:TPrimitive: phi(bb2:
y$21, bb3: y$21)
... ```
to ``` ... bb1 (block): predecessor blocks: bb2 bb3 x$36:TPrimitive:
phi(bb2: x$19, bb3: x$19) y$38:TPrimitive: phi(bb2: y$21, bb3: y$21) ... ```
Notice how `y`'s reassignment got skipped previously.
Previously if any operand was reactive, we transferred that reactivity to other
operands that had a mutable effect (capture, conditionally mutate, mutate, or
store). But a value can be captured without ever being modified again. This PR
updates the logic to only transfer reactivity among operands that are actually
mutable at the given instruction, based on the mutable range. This is strictly
more precise.
This PR adds one remaining feature to InferReactivePlaces: tracking indirections
like LoadLocal, PropertyLoad, and similar. Consider something like:
```
// INPUT
x.push(reactiveValue);
// HIR
t0 = LoadLocal 'x'
t1 = PropertyLoad t0, 'push'
t2 = LoadLocal 'reactiveValue' // reactive
t3 = CallExpression mutate t0 . read t1 ( read t2 )
```
Because a reactive value (`t2`) flows into `t0`, we want to record t0 as
reactive as well. But that's just the temporary for `LoadLocal 'x'` - what's
really happening is that from this point, `x` is reactive.
InferReactiveIdentifiers tracked this, and now that logic is ported into
InferReactivePlaces as well. That lets us remove all the actual inference from
InferReactiveIdentifiers.
Lauren Tan
Sathya Gunasekaran
Joe Savona