Noticed from our paste that we weren't correctly rolling up hoisting related
errors due to specific information being in the error title, so this PR moves
them into description instead.
Updates the approach used in ValidateNoSetStateInRender to detect function
expressions called during render. We now do the following:
* Track function expression which are known to unconditionally call setState
themselves- if these functions get called, that’s equivalent to calling
setState. We call the validation recursively to compute this.
* Track LoadLocal/StoreLocal indirections for such function expressions.
* Check CallExpressions where the callee is either a known SetState (via type
info) _or_ (new) where the callee is in the set of known-to-setState function
expressions.
The Set is shared throughout the analysis, so we can even find multiple levels
of indirection (see new test case).
I found an interesting edge case in the previous diff with mutation of a value
that appears in the expression of an object key:
```javascript
const key = {}
const object = {
[mutateAndReturnOtherValue(key)]: 42,
};
mutate(key);
```
We analyze and represent this correctly all the way through to codegen, but then
we hit the bug that @mofeiZ has noticed before: the temporary for `t =
mutateAndReturnOtherValue(key)` isn't emitted immediately (bc its a temporary).
It gets emitted inside the memo block for `object`, which is incorrect.
I tried to reproduce that here with JSX and it works as expected. It's an
interesting case though so let's land this to ensure we don't regress.
Adds a separate compiler flag for enabling the incomplete validation of ref
access within function expressions. Unlike the previous PR for
set-state-in-render validation, ref access in render can be okay in some
circumstances so i'm leaving this off by default. The point of splitting this up
is that our linting will be able to enable the rule without risk of false
positives.
The approach i initially took to validating function expressions was to try to
extend the mutable range if they are called during render, and then use the
mutable range of a function to determine if it's called during render later.
However there are cases where the range can be extended for other reasons, as
@poteto discovered, so we can't rely on the range extension. We've had several
of our validations completely off as a result of this.
In this PR i'm re-enabling @poteto's ValidateNoSetStateInRender pass by default,
but making the function expression checking use a separate compiler flag. This
means we'll have some false negatives, but should guarantee that we avoid false
positives. This means we can definitely catch things like:
```
const [state, setState] = useState(false);
setState(true);
```
Which we would have allowed by default before.
This reverts commit 10d129a8406e9d226abdb6943bf8512e34ce91db
---
Reverts #2311 due to undocumented assumptions being broken. I also added some
comments to `LoggerEvents` to explain each event type.
In `Program.ts`, we have something like the following code. `compile` could
produce any number of errors (not just expected errors / instances of
`CompilerError`). As an example, we sometimes error in `Codegen` due to babel
version incompatibilities (`Error: ObjectMethod: Too many arguments passed.
Received 7 but can receive no more than 5`).
```js
try {
// any error could be thrown here
compile(input);
} catch (e) {
// unknown type for e
handleError(e, ...);
}
```
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.
Sathya Gunasekaran
Lauren Tan
Joe Savona
Mofei Zhang