Skip to content
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

Potential repro for possible bug w #26763 #27465

Closed
wants to merge 1 commit into from

Conversation

josephsavona
Copy link
Contributor

@josephsavona josephsavona commented Oct 4, 2023

Possible bug w #26763. See comments for more. This is a simplified version of an internal test that failed after #26763, where we observed a different set of useEffects fired prior to taking a snapshot with ReactTestRenderer. This PR isn't a perfect repro, since manually reverting the changes from the above PR doesn't produce the expected result on this snapshot. But this is potentially in the ballpark of a repro, since the original code internally did fail and the expected result is different than expected in a similar way to the internal failure: a different set of useEffects runs as part of the act() call.

Related to #27463

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Oct 4, 2023
Comment on lines +202 to +209
<div
onClick={[Function]}
>
Expand
</div>,
<div>
Item 2
</div>,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

both items should be "expanded", so this should be showing <div>Item 1</div><div>Item 2</div>. But somehow only the useEffect from the second item takes effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rickhanlonii see comment here

@react-sizebot
Copy link

Comparing: 0fba3ec...9fe5904

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 174.46 kB 174.46 kB = 54.27 kB 54.27 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 176.31 kB 176.31 kB = 54.88 kB 54.88 kB
facebook-www/ReactDOM-prod.classic.js = 564.48 kB 564.48 kB = 99.37 kB 99.37 kB
facebook-www/ReactDOM-prod.modern.js = 548.21 kB 548.21 kB = 96.44 kB 96.44 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 9fe5904

const isExpanded = selectionContext.expanded.has(props.id);
const didInitiallyExpand = React.useRef(false);
const shouldInitiallyExpand =
props.initiallyExpanded && didInitiallyExpand.current === false;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

technically reading the ref here is wrong but it's easy to move it into the effect (and likely unrelated to the issue here)

@rickhanlonii
Copy link
Member

rickhanlonii commented Oct 5, 2023

When I run yarn test-www ReactTestRenderer-test.js --variant=false, the test passes. How can I see the assertion fail?

Looking through the CI failures, I see two reasons:

  • act(...) is not supported in production builds of React. (expected, TestRenderer doesn't have act in prod)
  • Expected test not to call console.warn(). (expected because the variant has the ref warning on, like @sophiebits pointed out)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants