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

Ensure updates are applied when diffInCommitPhase is on #26977

Merged
merged 2 commits into from
Jun 20, 2023

Conversation

sebmarkbage
Copy link
Collaborator

When we diffInCommitPhase there's no updatePayload, which caused no update to be applied.

This is unfortunate because it would've been a lot easier to see this oversight if we didn't have to support both flags.

I also carified that updateHostComponent is unnecessary in the new flag. We reuse updateHostComponent for HostSingleton and HostHoistables since it has a somewhat complex path but that means you have to remember when editing updateHostComponent that it's not just used for that tag. Luckily with the new flag, this is actually unnecessary since we just need to mark it for update if any props have changed and then we diff it later.

In this case there's no updatePayload.
We reuse updateHostComponent for HostSingleton and HostHoistables since it
has a somewhat complex path but that means you have to remember when editing
updateHostComponent that it's not just used for that tag.

Luckily with the new flag, this is actually unnecessary since we just need
to mark it for update if any props have changed and then we diff it later.
@sebmarkbage sebmarkbage requested review from gnoff and acdlite June 20, 2023 00:46
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jun 20, 2023
@react-sizebot
Copy link

Comparing: 613e6f5...b8a8b0a

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 = 164.56 kB 164.56 kB = 51.83 kB 51.83 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 172.00 kB 171.97 kB = 54.07 kB 54.06 kB
facebook-www/ReactDOM-prod.classic.js +0.06% 571.13 kB 571.47 kB +0.02% 100.75 kB 100.78 kB
facebook-www/ReactDOM-prod.modern.js +0.06% 554.91 kB 555.25 kB +0.03% 97.93 kB 97.96 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against b8a8b0a

@sebmarkbage sebmarkbage merged commit d1c8cda into facebook:main Jun 20, 2023
github-actions bot pushed a commit that referenced this pull request Jun 20, 2023
When we diffInCommitPhase there's no updatePayload, which caused no
update to be applied.

This is unfortunate because it would've been a lot easier to see this
oversight if we didn't have to support both flags.

I also carified that updateHostComponent is unnecessary in the new flag.
We reuse updateHostComponent for HostSingleton and HostHoistables since
it has a somewhat complex path but that means you have to remember when
editing updateHostComponent that it's not just used for that tag.
Luckily with the new flag, this is actually unnecessary since we just
need to mark it for update if any props have changed and then we diff it
later.

DiffTrain build for [d1c8cda](d1c8cda)
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
When we diffInCommitPhase there's no updatePayload, which caused no
update to be applied.

This is unfortunate because it would've been a lot easier to see this
oversight if we didn't have to support both flags.

I also carified that updateHostComponent is unnecessary in the new flag.
We reuse updateHostComponent for HostSingleton and HostHoistables since
it has a somewhat complex path but that means you have to remember when
editing updateHostComponent that it's not just used for that tag.
Luckily with the new flag, this is actually unnecessary since we just
need to mark it for update if any props have changed and then we diff it
later.
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
When we diffInCommitPhase there's no updatePayload, which caused no
update to be applied.

This is unfortunate because it would've been a lot easier to see this
oversight if we didn't have to support both flags.

I also carified that updateHostComponent is unnecessary in the new flag.
We reuse updateHostComponent for HostSingleton and HostHoistables since
it has a somewhat complex path but that means you have to remember when
editing updateHostComponent that it's not just used for that tag.
Luckily with the new flag, this is actually unnecessary since we just
need to mark it for update if any props have changed and then we diff it
later.

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