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

[flags] Cleanup enableCache #31778

Merged
merged 3 commits into from
Dec 15, 2024
Merged

[flags] Cleanup enableCache #31778

merged 3 commits into from
Dec 15, 2024

Conversation

rickhanlonii
Copy link
Member

This is landed everywhere

@vercel Vercel
Copy link

vercel bot commented Dec 14, 2024 •
edited
Loading

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 Dec 15, 2024 5:29pm

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Dec 14, 2024
@rickhanlonii rickhanlonii requested a review from acdlite December 14, 2024 17:06
@react-sizebot
Copy link

react-sizebot commented Dec 14, 2024 •
edited
Loading

Comparing: 2d32056...0d16aeb

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 +0.02% 510.68 kB 510.76 kB = 91.47 kB 91.36 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 = 515.62 kB 515.55 kB = 92.18 kB 92.17 kB
facebook-www/ReactDOM-prod.classic.js = 594.76 kB 594.69 kB = 104.95 kB 104.93 kB
facebook-www/ReactDOM-prod.modern.js = 585.03 kB 584.96 kB = 103.38 kB 103.37 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 7286add

@sebmarkbage
Copy link
Collaborator

I we used to have an enableCacheElement flag and I'm not sure when that was deleted. However, that flag was not properly flagging everything so now we're left which a bunch of dead code.

https://github.com/facebook/react/blob/main/packages/react-reconciler/src/ReactFiberCommitWork.js#L2906-L2920

We probably need to also remove the CacheComponent work.

@rickhanlonii
Copy link
Member Author

@sebmarkbage yeah I was going to ask about that because a ton of this looks unnecessary, do you thing I should followup to remove that dead code so I can revert it cleanly if needed, or just delete it in this PR?

sebmarkbage
sebmarkbage reviewed Dec 14, 2024
View reviewed changes
packages/react-reconciler/src/ReactFiberBeginWork.js
const cache: Cache = current.memoizedState.cache;
pushCacheProvider(workInProgress, cache);
}
const cache: Cache = current.memoizedState.cache;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need to fix these lints. Interesting that it wasn't failing for shadowing.

Copy link
Member Author

Choose a reason for hiding this comment

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

TIL case statements without braces do not scope let/const to that case, fixed by adding braces to the previous case.

@rickhanlonii
add braces to case
dc5e75a
@rickhanlonii
Merge branch 'main' into rh/rm-ff-cache
7286add 
# Conflicts:
#	packages/react-server/src/ReactFizzServer.js
@rickhanlonii rickhanlonii merged commit e06c72f into facebook:main Dec 15, 2024
187 checks passed
@rickhanlonii rickhanlonii deleted the rh/rm-ff-cache branch December 15, 2024 17:34
github-actions bot pushed a commit that referenced this pull request Dec 15, 2024
@rickhanlonii
[flags] Cleanup enableCache (#31778)
b2af848 
This is landed everywhere

DiffTrain build for [e06c72f](e06c72f)
github-actions bot pushed a commit that referenced this pull request Dec 15, 2024
@rickhanlonii
[flags] Cleanup enableCache (#31778)
fac54e0 
This is landed everywhere

DiffTrain build for [e06c72f](e06c72f)
github-actions bot pushed a commit to code/lib-react that referenced this pull request Dec 15, 2024
@pull
[flags] Cleanup enableCache (facebook#31778)
92ccdcf 
This is landed everywhere

DiffTrain build for [e06c72f](facebook@e06c72f)
github-actions bot pushed a commit to code/lib-react that referenced this pull request Dec 15, 2024
@pull
[flags] Cleanup enableCache (facebook#31778)
457b3f0 
This is landed everywhere

DiffTrain build for [e06c72f](facebook@e06c72f)
This was referenced Dec 18, 2024
Closed
Closed
Closed
Closed
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sebmarkbage sebmarkbage sebmarkbage approved these changes

@acdlite acdlite Awaiting requested review from acdlite

Assignees
No one assigned
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rickhanlonii @react-sizebot @sebmarkbage @facebook-github-bot