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.

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

fix(act): Don't warn if an effect was not queued #19319

Closed
wants to merge 7 commits into from
Closed

fix(act): Don't warn if an effect was not queued #19319

wants to merge 7 commits into from

Conversation

eps1lon
Copy link
Collaborator

@eps1lon eps1lon commented Jul 11, 2020 •
edited

Summary

Closes #19318

Test Plan

eps1lon
eps1lon commented Jul 11, 2020
View reviewed changes
packages/react-reconciler/src/ReactFiberHooks.new.js
@@ -1275,12 +1282,6 @@ function updateEffect(
create: () => (() => void) | void,
deps: Array<mixed> | void | null,
): void {
if (__DEV__) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should we move this in mountEffect as well for symmetry?

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 11, 2020 •
edited

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 49a479b:

Sandbox Source
React Configuration
missing act on every effect PR
missing act on every effect Issue #19318

@sizebot
Copy link

sizebot commented Jul 11, 2020 •
edited

Comparing: 2442d98...9a0e80e

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 = 127.41 kB 127.41 kB = 40.83 kB 40.83 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 130.22 kB 130.22 kB = 41.74 kB 41.74 kB
facebook-www/ReactDOM-prod.classic.js = 405.65 kB 405.65 kB = 74.99 kB 74.99 kB
facebook-www/ReactDOM-prod.modern.js = 394.08 kB 394.08 kB = 73.22 kB 73.22 kB
facebook-www/ReactDOMForked-prod.classic.js = 405.65 kB 405.65 kB = 75.00 kB 74.99 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 9a0e80e

@sizebot
Copy link

sizebot commented Jul 11, 2020 •
edited

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 49a479b

@eps1lon eps1lon marked this pull request as ready for review July 11, 2020 20:34
@eps1lon eps1lon force-pushed the fix/act-warn-effect-bailout branch from a03f286 to 72617dd Compare July 13, 2020 15:20
@stale
Copy link

stale bot commented Oct 12, 2020

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Oct 12, 2020
@eps1lon
Copy link
Collaborator Author

eps1lon commented Oct 12, 2020

Bump

@stale stale bot removed the Resolution: Stale Automatically closed due to inactivity label Oct 12, 2020
@eps1lon
Copy link
Collaborator Author

eps1lon commented Feb 12, 2021

Rebased and fixed some misconceptions about tags vs flags. I treated the hook flags as a single number when they're actually a bitset.

@eps1lon eps1lon force-pushed the fix/act-warn-effect-bailout branch from 6d1d5ca to 98ced8d Compare April 8, 2021 09:20
@eps1lon eps1lon changed the base branch from master to main June 30, 2021 12:31
@eps1lon
test(act): Add expected behavior for effects that were not queued
e110278
@eps1lon
fix(act): Don't warn if an effect was not queued
7b03ad3
@eps1lon
Explicitly expect no error
2175b00
@eps1lon
Add tests to verify that layout and ref effects cause no act warning
201f795
@eps1lon
sanity check -> confidence check
973efba
@eps1lon
Nest ifs instead of &&
6d66bd1 
Can't move FlowExpectError closer to the actual condition (like ts-expect-error).
In order to still get type-checking for the flag comparison we just wrap it in another  if.
Since this is dev-only the additional bytes should be fine.
In the end a compiler could always squash nested ifs.
@eps1lon eps1lon force-pushed the fix/act-warn-effect-bailout branch from 98ced8d to f4d0267 Compare June 30, 2021 12:42
@eps1lon eps1lon force-pushed the fix/act-warn-effect-bailout branch from f4d0267 to 9a0e80e Compare June 30, 2021 13:00
@eps1lon
Copy link
Collaborator Author

eps1lon commented Aug 11, 2021

Will no longer be relevant for React 18 where even a render needs to be wrapped in act. See reactwg/react-18#23 (reply in thread)

@eps1lon eps1lon closed this Aug 11, 2021
@eps1lon eps1lon deleted the fix/act-warn-effect-bailout branch August 11, 2021 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kbakdev kbakdev kbakdev approved these changes

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug: act warning misleading if an effect wasn't run
4 participants
@eps1lon @sizebot @kbakdev @facebook-github-bot