I experimented with more variations but prettier collapses most of them: any run
of consecutive whitespace within a jsxtext node will get collapsed into a single
space, for example. So the main difference in practice is whether a jsxtext node
(preceding a value) has a trailing space/newline or not:
```
Text <fbt:param /> Text
// vs
Text<fbt:param />Text
```
which we now have tests for.
Turns out this will cause an OOM in node.js when running eslint as part of the
IDE
``` Before: 16M packages/eslint-plugin-react-forget/dist/index.js After: 2.2M
packages/eslint-plugin-react-forget/dist/index.js ```
Anytime we have a nested `.scope` in code my brain hurts. For example
`scope.scope.dependencies`. This PR updates the scope merging pass to use the
name `scopeBlock` for a ReactiveScopeBlock and `scope` only for ReactiveScope
values, to make things a bit more clear.
Addresses T168684688 (#2242). MergeReactiveScopesThatInvalidateTogether does not
merge scopes if their output is not guaranteed to change when their inputs do.
So for example a case such as `{session_id: bar(props.bar)}` will not merge the
scopes for `t0 = bar(props.bar)` and `t1 = {session_id: t0}`, because t0 isn't
guaranteed to change when `props.bar` does, and we want to avoid recreating the
t1 object unless it semantically changes.
But there's a special case: if a scope has no dependencies, then we'll never
execute it again anyway. So it doesn't matter what kind of value it produces and
it's safe to merge with subsequent scopes:
```javascript
return {session_id: bar()}
```
Without the reactive input, `bar()` will always return the same value since
we'll only ever call it once anyway. So it's safe to then merge with the scope
for the outer object literal.
Current sprout output (if you remove the line in `SproutTodoFilter`):
```
Failures:
FAIL: bug-fbt-preserve-whitespace
Difference in forget and non-forget results.
Expected result: {
"kind": "ok",
"value": "Before text hello world",
"logs": []
}
Found: {
"kind": "ok",
"value": "Before texthello world",
"logs": []
}
```
`fbt` transforms run before jsx ones, so our lowering should also account for
[fbt's whitespace
rules](https://github.com/facebook/fbt/blob/main/packages/babel-plugin-fbt/src/fbt-nodes/FbtImplicitParamNode.js#L230-L233)
in `BuildHIR:trimJsxText`.
Fbt + typescript [seems](https://github.com/facebook/fbt/issues/49) [to
be](https://github.com/facebook/sfbt/issues/72) a non-blessed workflow. We do
want to allow for both flow and typescript tests, so I followed some
instructions [from a
guide](https://dev.to/retyui/how-to-add-support-typescript-for-fbt-an-internationalization-framework-3lo0)
to add support.
- fbt tags desugar to.. "fbt" strings. By default, typescript removes unused
imports at parse step (and on autoformat steps). We pass special
babel-typescript configs and change vscode settings to mitigate this.
- I tried adding `fbt` to the global scope, but the fbt transform asserts that
`fbt` is actually imported in the source program.
- Other hacks are available, like saying we'll only allow for fbt in flow files,
or always patching the source code to have an "fbt" import. This seemed the most
reasonable and easiest to debug / follow when writing tests
---
Refactor selection logic to be easier to read; add support for .jsx test files
(feels a bit weird adding a `.jsx` fixture and not seeing it get run)
In order to make changes to the testapp's dependencies, we need to update the
lockfile which means modules need to actually exist. This adds a new script to
just run a build of the packages we sync so that module resolution in the
testapp will work
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.
Sathya Gunasekaran
Joe Savona
Lauren Tan
Mofei Zhang