-
Notifications
You must be signed in to change notification settings - Fork 0
[compiler][e2e] Add rule of react violation fixture #60
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.
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
base: gh/mofeiZ/25/base
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,144 @@ | ||||||||||||||||||||||
| /** | ||||||||||||||||||||||
| * Copyright (c) Meta Platforms, Inc. and affiliates. | ||||||||||||||||||||||
| * | ||||||||||||||||||||||
| * This source code is licensed under the MIT license found in the | ||||||||||||||||||||||
| * LICENSE file in the root directory of this source tree. | ||||||||||||||||||||||
| */ | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| import {render, fireEvent} from '@testing-library/react'; | ||||||||||||||||||||||
| import * as React from 'react'; | ||||||||||||||||||||||
| import {useState} from 'react'; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| type Data = { | ||||||||||||||||||||||
| isDataFetched: boolean; | ||||||||||||||||||||||
| fieldData: { | ||||||||||||||||||||||
| username: { | ||||||||||||||||||||||
| errorMessage: string | null; | ||||||||||||||||||||||
| value: string | null; | ||||||||||||||||||||||
| }; | ||||||||||||||||||||||
| address: { | ||||||||||||||||||||||
| errorMessage: string | null; | ||||||||||||||||||||||
| value: string | null; | ||||||||||||||||||||||
| }; | ||||||||||||||||||||||
| }; | ||||||||||||||||||||||
| }; | ||||||||||||||||||||||
| function useRoRViolation(): [ | ||||||||||||||||||||||
| Data, | ||||||||||||||||||||||
| ( | ||||||||||||||||||||||
| field: 'username' | 'address', | ||||||||||||||||||||||
| kind: 'errorMessage' | 'value', | ||||||||||||||||||||||
| newValue: string, | ||||||||||||||||||||||
| ) => void, | ||||||||||||||||||||||
| ] { | ||||||||||||||||||||||
| 'use no forget'; | ||||||||||||||||||||||
| const [count, setCount] = useState<number>(0); | ||||||||||||||||||||||
| const [state, setState] = useState<Data>({ | ||||||||||||||||||||||
| isDataFetched: false, | ||||||||||||||||||||||
| fieldData: { | ||||||||||||||||||||||
| username: { | ||||||||||||||||||||||
| errorMessage: null, | ||||||||||||||||||||||
| value: null, | ||||||||||||||||||||||
| }, | ||||||||||||||||||||||
| address: { | ||||||||||||||||||||||
| errorMessage: null, | ||||||||||||||||||||||
| value: null, | ||||||||||||||||||||||
| }, | ||||||||||||||||||||||
| }, | ||||||||||||||||||||||
| }); | ||||||||||||||||||||||
| const update = ( | ||||||||||||||||||||||
| field: 'username' | 'address', | ||||||||||||||||||||||
| kind: 'errorMessage' | 'value', | ||||||||||||||||||||||
| ) => { | ||||||||||||||||||||||
| setState((prevState: Data) => { | ||||||||||||||||||||||
| const newState = {...prevState}; | ||||||||||||||||||||||
| const data = | ||||||||||||||||||||||
| field === 'username' | ||||||||||||||||||||||
| ? newState.fieldData.username | ||||||||||||||||||||||
| : newState.fieldData.address; | ||||||||||||||||||||||
| if (kind === 'errorMessage') { | ||||||||||||||||||||||
| data.errorMessage = `value${count}`; | ||||||||||||||||||||||
| } else { | ||||||||||||||||||||||
| data.errorMessage = `value${count}`; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
Comment on lines
+58
to
+62
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: both branches set
Suggested change
Prompt To Fix With AIThis is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/__tests__/e2e/rule-of-react-violations/inner-mutation-state.e2e.tsx
Line: 58:62
Comment:
**logic:** both branches set `data.errorMessage` - when `kind === 'value'`, should set `data.value` instead
```suggestion
if (kind === 'errorMessage') {
data.errorMessage = `value${count}`;
} else {
data.value = `value${count}`;
}
```
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||||||||
| return newState; | ||||||||||||||||||||||
| }); | ||||||||||||||||||||||
| setCount(count + 1); | ||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: stale closure issue - Prompt To Fix With AIThis is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/__tests__/e2e/rule-of-react-violations/inner-mutation-state.e2e.tsx
Line: 65:65
Comment:
**logic:** stale closure issue - `count` value used on line 59/61 will always be the value from when `update` was created, not the current state. Should use the functional form: `setCount(prevCount => prevCount + 1)` and access count from the updater, or use `count` from within the setState callback
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||||||||
| }; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| return [state, update]; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| function Component({ | ||||||||||||||||||||||
| label, | ||||||||||||||||||||||
| showFields, | ||||||||||||||||||||||
| }: { | ||||||||||||||||||||||
| label: keyof Data['fieldData']; | ||||||||||||||||||||||
| showFields: boolean; | ||||||||||||||||||||||
| }) { | ||||||||||||||||||||||
| const [state, update] = useRoRViolation(); | ||||||||||||||||||||||
| function readState(label: keyof Data['fieldData']) { | ||||||||||||||||||||||
| return state.fieldData[label]; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| const field = readState(label); | ||||||||||||||||||||||
| return ( | ||||||||||||||||||||||
| <> | ||||||||||||||||||||||
| {showFields ? ( | ||||||||||||||||||||||
| <div> | ||||||||||||||||||||||
| {field.errorMessage ?? '(none)'} {field.value ?? '(none)'} | ||||||||||||||||||||||
| </div> | ||||||||||||||||||||||
| ) : ( | ||||||||||||||||||||||
| <div>no data</div> | ||||||||||||||||||||||
| )} | ||||||||||||||||||||||
| <button | ||||||||||||||||||||||
| data-testid="state_update" | ||||||||||||||||||||||
| onClick={() => update(label, 'errorMessage', 'new value')}></button> | ||||||||||||||||||||||
| </> | ||||||||||||||||||||||
| ); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| test('update-button', () => { | ||||||||||||||||||||||
| const label: 'username' = 'username'; | ||||||||||||||||||||||
| const {asFragment, rerender, getByTestId} = render( | ||||||||||||||||||||||
| <Component label={label} showFields={true} />, | ||||||||||||||||||||||
| ); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| expect(asFragment()).toMatchInlineSnapshot(` | ||||||||||||||||||||||
| <DocumentFragment> | ||||||||||||||||||||||
| <div> | ||||||||||||||||||||||
| (none) (none) | ||||||||||||||||||||||
| </div> | ||||||||||||||||||||||
| <button | ||||||||||||||||||||||
| data-testid="state_update" | ||||||||||||||||||||||
| /> | ||||||||||||||||||||||
| </DocumentFragment> | ||||||||||||||||||||||
| `); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| fireEvent.click(getByTestId('state_update')); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if (__FORGET__) { | ||||||||||||||||||||||
| /** | ||||||||||||||||||||||
| * Now we have a reactivity bug due to a rule-of-react violation! | ||||||||||||||||||||||
| */ | ||||||||||||||||||||||
| expect(asFragment()).toMatchInlineSnapshot(` | ||||||||||||||||||||||
| <DocumentFragment> | ||||||||||||||||||||||
| <div> | ||||||||||||||||||||||
| (none) (none) | ||||||||||||||||||||||
| </div> | ||||||||||||||||||||||
| <button | ||||||||||||||||||||||
| data-testid="state_update" | ||||||||||||||||||||||
| /> | ||||||||||||||||||||||
| </DocumentFragment> | ||||||||||||||||||||||
| `); | ||||||||||||||||||||||
| } else { | ||||||||||||||||||||||
| expect(asFragment()).toMatchInlineSnapshot(` | ||||||||||||||||||||||
| <DocumentFragment> | ||||||||||||||||||||||
| <div> | ||||||||||||||||||||||
| value0 (none) | ||||||||||||||||||||||
| </div> | ||||||||||||||||||||||
| <button | ||||||||||||||||||||||
| data-testid="state_update" | ||||||||||||||||||||||
| /> | ||||||||||||||||||||||
| </DocumentFragment> | ||||||||||||||||||||||
| `); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| }); | ||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
syntax: function signature mismatch - the type definition on line 30 declares a third parameter
newValue: string, but the implementation doesn't accept it. Either removenewValuefrom the type or add it to the function parameters.Prompt To Fix With AI