Skip to content

disableInputAttributeSyncing #2

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

disableInputAttributeSyncing #2

wants to merge 2 commits into from

Conversation

@kassens
Copy link
Owner

@kassens kassens commented Nov 18, 2022

@kassens kassens closed this Nov 19, 2022
@kassens kassens deleted the disableInputAttributeSyncing branch March 19, 2024 19:02
kassens pushed a commit that referenced this pull request Jun 4, 2024
@mofeiZ
[babel][contextvar] Patch context identifier babel logic; only use re…
241a615 
…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; 

} 

} 

```
kassens pushed a commit that referenced this pull request Jun 11, 2025
@wlemahieu
[compiler]: fix link compiler & 4 broken tests from path containing s…
b6c0aa8 
…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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kassens @colinhacks