Add renderer mode flags for dead code elimination #2
Conversation
This file contains hidden or 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
sebmarkbage
reviewed
Oct 18, 2017
src/renderers/noop/ReactNoopEntry.js
Outdated
| }); | ||
| }; | ||
|
|
||
| // They are created lazily because only one can be created per test file. |
There was a problem hiding this comment.
I thought you solved this? Why can't both modes be enabled?
There was a problem hiding this comment.
I think it's just because I would have to enable persistent reconciler for every test using Noop. I could create only the persistent one lazily.
There was a problem hiding this comment.
Or I could enable it everywhere where Noop is used though I feel lazy about this.
There was a problem hiding this comment.
Why not just enable it in all tests by changing the default mock?
There was a problem hiding this comment.
Want to keep ReactDOM test paths more realistic.
|
Simpler fix coming. |
sebmarkbage
pushed a commit
that referenced
this pull request
Sep 5, 2019
Fix encoding of Unicode keys greater than U+00FF
sebmarkbage
pushed a commit
that referenced
this pull request
May 17, 2024
…ferenced
identifiers
---
A few fixes for finding context identifiers:
Previously, we counted every babel identifier as a reference. This is
problematic because babel counts every string symbol as an identifier.
```js
print(x); // x is an identifier as expected
obj.x // x is.. also an identifier here
{x: 2} // x is also an identifier here
```
This PR adds a check for `isReferencedIdentifier`. Note that only non-lval
references pass this check
```js
print(x); // isReferencedIdentifier(x) -> true
obj.x // isReferencedIdentifier(x) -> false
{x: 2} // isReferencedIdentifier(x) -> false
x = 2 // isReferencedIdentifier(x) -> false
```
Which brings us to change #2.
Previously, we counted assignments as references due to the identifier visiting
+ checking logic. The logic was roughly the following (from facebook#1691)
```js
contextVars = intersection(reassigned, referencedByInnerFn);
```
Now that assignments (lvals) and references (rvals) are tracked separately, the
equivalent logic is this. Note that assignment to a context variable does not
need to be modeled as a read (`console.log(x = 5)` always will evaluates and
prints 5, regardless of the previous value of x).
```
contextVars = union(reassignedByInnerFn, intersection(reassigned,
referencedByInnerFn))
```
---
Note that variables that are never read do not need to be modeled as context
variables, but this is unlikely to be a common pattern.
```js
function fn() {
let x = 2;
const inner = () => {
x = 3;
}
}
```
sebmarkbage
pushed a commit
that referenced
this pull request
Jun 10, 2025
…paces (facebook#33409) ## Summary Problem #1: Running the `link-compiler.sh` bash script via `"prebuild"` script fails if a developer has cloned the `react` repo into a folder that contains _any_ spaces. 3 tests fail because of this. <img width="1003" alt="fail-1" src="https://github.com/user-attachments/assets/1fbfa9ce-4f84-48d7-b49c-b6e967b8c7ca" /> <img width="1011" alt="fail-2" src="https://github.com/user-attachments/assets/0a8c6371-a2df-4276-af98-38f4784cf0da" /> <img width="1027" alt="fail-3" src="https://github.com/user-attachments/assets/1c4f4429-800c-4b44-b3da-a59ac85a16b9" /> For example, my current folder is: `/Users/wes/Development/Open Source Contributions/react` The link compiler error returns: `./scripts/react-compiler/link-compiler.sh: line 15: cd: /Users/wes/Development/Open: No such file or directory` Problem #2: 1 test in `ReactChildren-test.js` fails due the existing stack trace regex which should be lightly revised. `([^(\[\n]+)[^\n]*/g` is more robust for stack traces: it captures the function/class name (with dots) and does not break on spaces in file paths. `([\S]+)[^\n]*/g` is simpler but breaks if there are spaces and doesn't handle dotted names well. Additionally, we trim the whitespace off the name to resolve extra spaces breaking this test as well: ``` - in div (at **) + in div (at **) ``` <img width="987" alt="fail-4" src="https://github.com/user-attachments/assets/56a673bc-513f-4458-95b2-224129c77144" /> All of the above tests pass if I hyphenate my local folder: `/Users/wes/Development/Open-Source-Contributions/react` I selfishly want to keep spaces in my folder names. 🫣 ## How did you test this change? **npx yarn prebuild** Before: <img width="896" alt="Screenshot at Jun 01 11-42-56" src="https://github.com/user-attachments/assets/4692775c-1e5c-4851-9bd7-e12ed5455e47" /> After: <img width="420" alt="Screenshot at Jun 01 11-43-42" src="https://github.com/user-attachments/assets/4e303c00-02b7-4540-ba19-927b2d7034fb" /> **npx yarn test** **npx yarn test ./packages/react/src/\_\_tests\_\_/ReactChildren-test.js** **npx yarn test -r=xplat --env=development --variant=true --ci --shard=3/5** Before: <img width="438" alt="before" src="https://github.com/user-attachments/assets/f5eedb22-18c3-4124-a04b-daa95c0f7652" /> After: <img width="439" alt="after" src="https://github.com/user-attachments/assets/a94218ba-7c6a-4f08-85d3-57540e9d0029" /> <img width="650" alt="Screenshot at Jun 02 18-03-39" src="https://github.com/user-attachments/assets/3eae993c-a56b-46c8-ae02-d249cb053fe7" /> <img width="685" alt="Screenshot at Jun 03 12-53-47" src="https://github.com/user-attachments/assets/5b2caa33-d3dc-4804-981d-52cb10b6226f" />
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
View the diff with
?w.https://github.com/sebmarkbage/react/pull/2/files?w=1