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

Reconciler: Change commitUpdate signature to account for unused updatePayload parameter #28909

Merged

Conversation

eps1lon
Copy link
Collaborator

@eps1lon eps1lon commented Apr 25, 2024

diffInCommitPhase is shipped (#27409) so we don't need to pass along updatePayload which is always null. react-three-fiber was bailing out if this payload was null and therefore never committed any update.

By changing the signature the breakage becomes more obvious. The docs already document the proposed signature: https://github.com/facebook/react/blob/main/packages/react-reconciler/README.md#commitupdateinstance-type-prevprops-nextprops-internalhandle

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Apr 25, 2024
@react-sizebot
Copy link

react-sizebot commented Apr 25, 2024

Comparing: cf5ab8b...34fd689

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.js = 6.66 kB 6.66 kB +0.05% 1.82 kB 1.82 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.67 kB 6.67 kB +0.11% 1.83 kB 1.83 kB
facebook-www/ReactDOM-prod.classic.js = 591.14 kB 591.05 kB = 103.91 kB 103.90 kB
facebook-www/ReactDOM-prod.modern.js = 567.36 kB 567.27 kB = 100.30 kB 100.30 kB
test_utils/ReactAllWarnings.js Deleted 64.26 kB 0.00 kB Deleted 16.02 kB 0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-experimental/react-noop-renderer/cjs/react-noop-renderer-persistent.production.js = 35.17 kB 35.08 kB = 6.58 kB 6.57 kB
oss-stable-semver/react-noop-renderer/cjs/react-noop-renderer-persistent.production.js = 35.17 kB 35.08 kB = 6.58 kB 6.57 kB
oss-stable/react-noop-renderer/cjs/react-noop-renderer-persistent.production.js = 35.17 kB 35.08 kB = 6.58 kB 6.57 kB
oss-experimental/react-noop-renderer/cjs/react-noop-renderer.production.js = 35.04 kB 34.95 kB = 6.56 kB 6.55 kB
oss-stable-semver/react-noop-renderer/cjs/react-noop-renderer.production.js = 35.04 kB 34.95 kB = 6.56 kB 6.55 kB
oss-stable/react-noop-renderer/cjs/react-noop-renderer.production.js = 35.04 kB 34.95 kB = 6.56 kB 6.55 kB
test_utils/ReactAllWarnings.js Deleted 64.26 kB 0.00 kB Deleted 16.02 kB 0.00 kB

Generated by 🚫 dangerJS against 34fd689

@eps1lon eps1lon force-pushed the cleanup-diffInCommit-unused-updatePayload branch 2 times, most recently from 3db92a6 to 2989591 Compare April 25, 2024 14:56
// We may have an update on a Hoistable element
const updatePayload: null | UpdatePayload =
(finishedWork.updateQueue: any);
finishedWork.updateQueue = null;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Confirmed by logging in yarn test that finishedWork.updateQueu is always null here

// TODO: Type the updateQueue to be specific to host components.
const updatePayload: null | UpdatePayload =
(finishedWork.updateQueue: any);
finishedWork.updateQueue = null;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Confirmed by logging in yarn test that finishedWork.updateQueu is always null here

Copy link
Contributor

@jackpope jackpope left a comment

Choose a reason for hiding this comment

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

One lint issue to fix

@eps1lon eps1lon force-pushed the cleanup-diffInCommit-unused-updatePayload branch from 2989591 to 34fd689 Compare April 25, 2024 17:06
@eps1lon eps1lon merged commit 82d8129 into facebook:main Apr 25, 2024
38 checks passed
@eps1lon eps1lon deleted the cleanup-diffInCommit-unused-updatePayload branch April 25, 2024 17:14
bigfootjon pushed a commit that referenced this pull request Apr 25, 2024
…datePayload` parameter (#28909)

DiffTrain build for commit 82d8129.
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.

4 participants