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

Cleanup enableUseRefAccessWarning flag #28699

Merged
merged 1 commit into from
Apr 3, 2024
Merged

Conversation

kassens
Copy link
Member

@kassens kassens commented Apr 1, 2024

Cleanup enableUseRefAccessWarning flag

I don't think this flag has a path forward in the current implementation. The detection by stack trace is too brittle to detect the lazy initialization pattern reliably (see e.g. some internal tests that expect the warning because they use lazy intialization, but a slightly different pattern then the expected pattern.

I think a new version of this could be to fully ban ref access during render with an alternative API for the exceptional cases that today require ref access during render.

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

react-sizebot commented Apr 1, 2024

Comparing: 7a2609e...ab84793

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 = 169.31 kB 169.31 kB = 52.76 kB 52.76 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 171.13 kB 171.13 kB = 53.33 kB 53.33 kB
facebook-www/ReactDOM-prod.classic.js = 589.03 kB 588.80 kB = 103.68 kB 103.63 kB
facebook-www/ReactDOM-prod.modern.js = 566.05 kB 565.82 kB = 99.59 kB 99.54 kB
test_utils/ReactAllWarnings.js Deleted 64.60 kB 0.00 kB Deleted 16.14 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
react-native/implementations/ReactNativeRenderer-dev.fb.js = 1,113.39 kB 1,110.24 kB = 221.75 kB 221.00 kB
react-native/implementations/ReactFabric-dev.fb.js = 1,098.78 kB 1,095.62 kB = 217.90 kB 217.16 kB
facebook-www/ReactART-dev.classic.js = 1,081.18 kB 1,078.02 kB = 211.46 kB 210.72 kB
facebook-www/ReactART-dev.modern.js = 1,047.53 kB 1,044.36 kB = 205.36 kB 204.63 kB
test_utils/ReactAllWarnings.js Deleted 64.60 kB 0.00 kB Deleted 16.14 kB 0.00 kB

Generated by 🚫 dangerJS against ab84793

Copy link
Member

@rickhanlonii rickhanlonii left a comment

Choose a reason for hiding this comment

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

I think we still want this?

Comment on lines -860 to -861
// The selector implementation uses the lazy ref initialization pattern
// @gate !(enableUseRefAccessWarning && __DEV__)
Copy link
Member Author

Choose a reason for hiding this comment

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

See example this was opted out, because it supposedly uses the "lazy ref init pattern", so the checks that try to allow that are failing here.

@kassens
Copy link
Member Author

kassens commented Apr 1, 2024

The feature flag was basically untouched since it was added and didn't have an owner in years.

I briefly tried to enable it, but the detection was way to unreliable and I don't see a path forward for the current approach.

I don't think this flag has a path forward in the current implementation. The detection by stack trace is too brittle to detect the lazy initialization pattern reliably (see e.g. some internal tests that expect the warning because they use lazy intialization, but a slightly different pattern then the expected pattern.

I think a new version of this could be to fully ban ref access during render with an alternative API for the exceptional cases that today require ref access during render.
Copy link
Member

@rickhanlonii rickhanlonii left a comment

Choose a reason for hiding this comment

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

Ok let's remove

@kassens kassens merged commit 20e710a into facebook:main Apr 3, 2024
38 checks passed
@kassens kassens deleted the pr28699 branch April 3, 2024 17:35
github-actions bot pushed a commit that referenced this pull request Apr 3, 2024
Cleanup enableUseRefAccessWarning flag

I don't think this flag has a path forward in the current
implementation. The detection by stack trace is too brittle to detect
the lazy initialization pattern reliably (see e.g. some internal tests
that expect the warning because they use lazy intialization, but a
slightly different pattern then the expected pattern.

I think a new version of this could be to fully ban ref access during
render with an alternative API for the exceptional cases that today
require ref access during render.

DiffTrain build for [20e710a](20e710a)
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
Cleanup enableUseRefAccessWarning flag

I don't think this flag has a path forward in the current
implementation. The detection by stack trace is too brittle to detect
the lazy initialization pattern reliably (see e.g. some internal tests
that expect the warning because they use lazy intialization, but a
slightly different pattern then the expected pattern.

I think a new version of this could be to fully ban ref access during
render with an alternative API for the exceptional cases that today
require ref access during render.
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
Cleanup enableUseRefAccessWarning flag

I don't think this flag has a path forward in the current
implementation. The detection by stack trace is too brittle to detect
the lazy initialization pattern reliably (see e.g. some internal tests
that expect the warning because they use lazy intialization, but a
slightly different pattern then the expected pattern.

I think a new version of this could be to fully ban ref access during
render with an alternative API for the exceptional cases that today
require ref access during render.

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