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

Turn on string ref deprecation warning for everybody (codemoddable) #25334

Closed

Conversation

eps1lon
Copy link
Collaborator

@eps1lon eps1lon commented Sep 27, 2022

Summary

For reactjs/rfcs@createlement-rfc/text/0000-create-element-changes.md#deprecate-string-refs-and-remove-production-mode-_owner-field

Enable warnAboutStringRefs for every release.
I adjusted the warning logic to not also appear in other cases where we already warn/throw (e.g. function components cannot be given refs).

For the string-refs codemod

ref={(current) => {
  this.refs.stringRef = current;
}}

to work, we now have to lazily initialize this.refs for every function ref.

Alternatively we could do

ref={(current) => {
  if (this.refs === emptyRefsObject) {
    // This is a lazy pooled frozen object, so we need to initialize.
    this.refs = {};
  }
  if (current === null) {
    delete this.refs.stringRef;
  } else {
    this.refs.stringRef = current;
  }
}}

But this gets increasingly difficult since we now have to add an import and global initializer instead of only adjust the local ref.

Or we always eagerly initialize this.refs when constructiong class instances.
Or we don't add any codemod: #25383

How did you test this change?

  • CI

@sizebot
Copy link

sizebot commented Sep 27, 2022

Comparing: 513417d...e19c22c

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 +0.19% 135.47 kB 135.72 kB +0.15% 43.44 kB 43.50 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js +0.17% 148.01 kB 148.26 kB +0.13% 47.33 kB 47.40 kB
facebook-www/ReactDOM-prod.classic.js +0.11% 492.51 kB 493.07 kB +0.11% 87.52 kB 87.62 kB
facebook-www/ReactDOM-prod.modern.js +0.12% 477.80 kB 478.36 kB +0.12% 85.29 kB 85.39 kB
facebook-www/ReactDOMForked-prod.classic.js +0.11% 492.51 kB 493.07 kB +0.11% 87.52 kB 87.62 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable-semver/react-art/cjs/react-art.production.min.js +0.29% 85.29 kB 85.54 kB +0.24% 26.47 kB 26.53 kB
oss-stable/react-art/cjs/react-art.production.min.js +0.29% 85.31 kB 85.56 kB +0.25% 26.47 kB 26.54 kB
oss-experimental/react-art/cjs/react-art.production.min.js +0.27% 91.96 kB 92.21 kB +0.22% 28.30 kB 28.36 kB
oss-stable-semver/react-test-renderer/cjs/react-test-renderer.production.min.js +0.27% 92.60 kB 92.85 kB +0.25% 28.55 kB 28.62 kB
oss-stable/react-test-renderer/cjs/react-test-renderer.production.min.js +0.27% 92.63 kB 92.88 kB +0.26% 28.55 kB 28.62 kB
oss-stable-semver/react-test-renderer/umd/react-test-renderer.production.min.js +0.27% 92.85 kB 93.10 kB +0.26% 29.01 kB 29.08 kB
oss-stable/react-test-renderer/umd/react-test-renderer.production.min.js +0.27% 92.87 kB 93.13 kB +0.26% 29.01 kB 29.08 kB
oss-stable-semver/react-reconciler/cjs/react-reconciler.production.min.js +0.26% 96.80 kB 97.05 kB +0.18% 29.74 kB 29.80 kB
oss-stable/react-reconciler/cjs/react-reconciler.production.min.js +0.26% 96.83 kB 97.08 kB +0.17% 29.76 kB 29.82 kB
oss-experimental/react-test-renderer/cjs/react-test-renderer.production.min.js +0.26% 97.34 kB 97.59 kB +0.22% 29.88 kB 29.94 kB
oss-experimental/react-test-renderer/umd/react-test-renderer.production.min.js +0.26% 97.58 kB 97.83 kB +0.25% 30.28 kB 30.36 kB
oss-experimental/react-reconciler/cjs/react-reconciler.production.min.js +0.24% 104.29 kB 104.54 kB +0.18% 31.83 kB 31.89 kB
oss-stable-semver/react-reconciler/cjs/react-reconciler.profiling.min.js +0.24% 105.63 kB 105.88 kB +0.22% 31.95 kB 32.02 kB
oss-stable/react-reconciler/cjs/react-reconciler.profiling.min.js +0.24% 105.65 kB 105.90 kB +0.21% 31.97 kB 32.04 kB
oss-experimental/react-reconciler/cjs/react-reconciler.profiling.min.js +0.22% 113.14 kB 113.39 kB +0.19% 33.98 kB 34.04 kB
oss-stable-semver/react-art/umd/react-art.production.min.js +0.21% 121.17 kB 121.42 kB +0.18% 37.69 kB 37.76 kB
oss-stable/react-art/umd/react-art.production.min.js +0.21% 121.19 kB 121.44 kB +0.18% 37.69 kB 37.76 kB
facebook-react-native/react-test-renderer/cjs/ReactTestRenderer-prod.js +0.20% 280.80 kB 281.36 kB +0.21% 49.73 kB 49.83 kB
oss-stable-semver/react-test-renderer/cjs/react-test-renderer.development.js +0.20% 691.48 kB 692.87 kB +0.15% 150.21 kB 150.43 kB
oss-stable/react-test-renderer/cjs/react-test-renderer.development.js +0.20% 691.51 kB 692.90 kB +0.15% 150.23 kB 150.45 kB

Generated by 🚫 dangerJS against e19c22c

@eps1lon eps1lon force-pushed the forwardRef/deprecate-string-refs branch 2 times, most recently from 7c58a47 to e93410f Compare September 27, 2022 15:17
@eps1lon eps1lon marked this pull request as ready for review September 27, 2022 15:21
@eps1lon eps1lon requested a review from sebmarkbage September 27, 2022 15:22
@eps1lon eps1lon changed the title Turn on string ref deprecation warning for everybody Turn on string ref deprecation warning for everybody (codemoddable) Oct 9, 2022
@eps1lon eps1lon force-pushed the forwardRef/deprecate-string-refs branch from e93410f to e19c22c Compare October 9, 2022 09:11
@sebmarkbage
Copy link
Collaborator

I think the move here is to always initialize a new object for classes. The object pooling is a micro-optimization that doesn't matter in the grand scheme of things.

Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

Choose a reason for hiding this comment

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

Let's do #25383 and #25696 isntead.

sebmarkbage added a commit that referenced this pull request Nov 16, 2022
This micro-optimization never made sense and less so now that they're
rare.

This still initializes the class with a shared immutable object in the
constructor - which is also what createClass() does.

Then we override it during mount. This is done in case someone messes up
the initialization of the super() constructor for example, which was
more common in polyfills.

This change means that if a ref is initialized during the constructor
itself it wouldn't be lazily initialized but that's not user code that
does it, it's React so that shouldn't happen.

This makes string refs codemoddable as described in.
#25334
sebmarkbage pushed a commit that referenced this pull request Nov 17, 2022
…e) (#25383)

## Summary
 
Alternate to #25334 without any
prod runtime changes i.e. the proposed codemod in
https://github.com/reactjs/rfcs/blob/createlement-rfc/text/0000-create-element-changes.md#deprecate-string-refs-and-remove-production-mode-_owner-field
would not work.

## How did you test this change?

- [x] CI
- [x] `yarn test` with and without `warnAboutStringRefs`
@eps1lon
Copy link
Collaborator Author

eps1lon commented Nov 17, 2022

Replaced by #25383

@eps1lon eps1lon closed this Nov 17, 2022
@eps1lon eps1lon deleted the forwardRef/deprecate-string-refs branch November 17, 2022 09:12
mofeiZ pushed a commit to mofeiZ/react that referenced this pull request Nov 17, 2022
This micro-optimization never made sense and less so now that they're
rare.

This still initializes the class with a shared immutable object in the
constructor - which is also what createClass() does.

Then we override it during mount. This is done in case someone messes up
the initialization of the super() constructor for example, which was
more common in polyfills.

This change means that if a ref is initialized during the constructor
itself it wouldn't be lazily initialized but that's not user code that
does it, it's React so that shouldn't happen.

This makes string refs codemoddable as described in.
facebook#25334
mofeiZ pushed a commit to mofeiZ/react that referenced this pull request Nov 17, 2022
…e) (facebook#25383)

## Summary
 
Alternate to facebook#25334 without any
prod runtime changes i.e. the proposed codemod in
https://github.com/reactjs/rfcs/blob/createlement-rfc/text/0000-create-element-changes.md#deprecate-string-refs-and-remove-production-mode-_owner-field
would not work.

## How did you test this change?

- [x] CI
- [x] `yarn test` with and without `warnAboutStringRefs`
rickhanlonii pushed a commit that referenced this pull request Dec 3, 2022
This micro-optimization never made sense and less so now that they're
rare.

This still initializes the class with a shared immutable object in the
constructor - which is also what createClass() does.

Then we override it during mount. This is done in case someone messes up
the initialization of the super() constructor for example, which was
more common in polyfills.

This change means that if a ref is initialized during the constructor
itself it wouldn't be lazily initialized but that's not user code that
does it, it's React so that shouldn't happen.

This makes string refs codemoddable as described in.
#25334
rickhanlonii pushed a commit that referenced this pull request Dec 3, 2022
…e) (#25383)

## Summary
 
Alternate to #25334 without any
prod runtime changes i.e. the proposed codemod in
https://github.com/reactjs/rfcs/blob/createlement-rfc/text/0000-create-element-changes.md#deprecate-string-refs-and-remove-production-mode-_owner-field
would not work.

## How did you test this change?

- [x] CI
- [x] `yarn test` with and without `warnAboutStringRefs`
jerrydev0927 added a commit to jerrydev0927/react that referenced this pull request Jan 5, 2024
This micro-optimization never made sense and less so now that they're
rare.

This still initializes the class with a shared immutable object in the
constructor - which is also what createClass() does.

Then we override it during mount. This is done in case someone messes up
the initialization of the super() constructor for example, which was
more common in polyfills.

This change means that if a ref is initialized during the constructor
itself it wouldn't be lazily initialized but that's not user code that
does it, it's React so that shouldn't happen.

This makes string refs codemoddable as described in.
facebook/react#25334
jerrydev0927 added a commit to jerrydev0927/react that referenced this pull request Jan 5, 2024
…e) (#25383)

## Summary
 
Alternate to facebook/react#25334 without any
prod runtime changes i.e. the proposed codemod in
https://github.com/reactjs/rfcs/blob/createlement-rfc/text/0000-create-element-changes.md#deprecate-string-refs-and-remove-production-mode-_owner-field
would not work.

## How did you test this change?

- [x] CI
- [x] `yarn test` with and without `warnAboutStringRefs`
acdlite pushed a commit that referenced this pull request Apr 16, 2024
…e) (#25383)

## Summary
 
Alternate to #25334 without any
prod runtime changes i.e. the proposed codemod in
https://github.com/reactjs/rfcs/blob/createlement-rfc/text/0000-create-element-changes.md#deprecate-string-refs-and-remove-production-mode-_owner-field
would not work.

## How did you test this change?

- [x] CI
- [x] `yarn test` with and without `warnAboutStringRefs`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants