KEMBAR78
Ensure updates are applied when diffInCommitPhase is on by sebmarkbage · Pull Request #26977 · facebook/react · GitHub
Skip to content

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 acdlite and gnoff 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