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

[crud] Fix deps comparison bug #31599

Merged
merged 1 commit into from
Nov 20, 2024
Merged

[crud] Fix deps comparison bug #31599

merged 1 commit into from
Nov 20, 2024

Conversation

poteto
Copy link
Member

@poteto poteto commented Nov 20, 2024

Fixes a bug with the experimental useResourceEffect hook where we would compare the wrong deps when there happened to be another kind of effect preceding the ResourceEffect. To do this correctly we need to add a pointer to the ResourceEffect's identity on the update.

I also unified the previously separate push effect impls for resource effects since they are always pushed together as a unit.

Copy link

vercel bot commented Nov 20, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-compiler-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 20, 2024 7:37pm

Fixes a bug with the experimental `useResourceEffect` hook where we would compare the wrong deps when there happened to be another kind of effect preceding the ResourceEffect. To do this correctly we need to add a pointer to the ResourceEffect's identity on the update.

I also unified the previously separate push effect impls for resource effects since they are always pushed together as a unit.
@react-sizebot
Copy link

react-sizebot commented Nov 20, 2024

Comparing: 64f8951...50ba87b

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.68 kB 6.68 kB = 1.83 kB 1.83 kB
oss-stable/react-dom/cjs/react-dom-client.production.js = 509.94 kB 509.94 kB = 91.24 kB 91.24 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.69 kB 6.69 kB = 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js = 514.88 kB 514.88 kB = 91.95 kB 91.95 kB
facebook-www/ReactDOM-prod.classic.js +0.04% 594.55 kB 594.80 kB +0.06% 104.90 kB 104.96 kB
facebook-www/ReactDOM-prod.modern.js +0.04% 584.81 kB 585.07 kB +0.06% 103.32 kB 103.39 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
test_utils/ReactAllWarnings.js = 63.51 kB 63.39 kB = 15.83 kB 15.81 kB

Generated by 🚫 dangerJS against d6c45a7

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.

Discussed offline. The two-effect design still needs work, but we have a better understanding of what needs to change now, and this PR fixes a major bug blocking further experimentation.

@poteto poteto merged commit c11c951 into main Nov 20, 2024
184 checks passed
@poteto poteto deleted the pr31599 branch November 20, 2024 21:54
github-actions bot pushed a commit that referenced this pull request Nov 20, 2024
Fixes a bug with the experimental `useResourceEffect` hook where we
would compare the wrong deps when there happened to be another kind of
effect preceding the ResourceEffect. To do this correctly we need to add
a pointer to the ResourceEffect's identity on the update.

I also unified the previously separate push effect impls for resource
effects since they are always pushed together as a unit.

DiffTrain build for [c11c951](c11c951)
github-actions bot pushed a commit that referenced this pull request Nov 20, 2024
Fixes a bug with the experimental `useResourceEffect` hook where we
would compare the wrong deps when there happened to be another kind of
effect preceding the ResourceEffect. To do this correctly we need to add
a pointer to the ResourceEffect's identity on the update.

I also unified the previously separate push effect impls for resource
effects since they are always pushed together as a unit.

DiffTrain build for [c11c951](c11c951)
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