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

unify deprecated/unsafe lifecycle warnings, pass tests #16103

Merged
merged 1 commit into from Jul 15, 2019

Conversation

@threepointone
Copy link
Contributor

commented Jul 10, 2019

This PR tweaks the deprecated/unsafe lifecycle warnings.

  • redoes #15431 from scratch, taking on the feedback there
  • unifies the messaging between "deprecated" and UNSAFE_ lifecycle messages. It reorganises ReactStrictModeWarnings.js to capture and flush all the lifecycle warnings in one procedure each.
  • this DOES NOT do the above treatment for context warnings, I'll do that in another PR too
  • matches the warning from ReactPartialRenderer to match the above treatment
  • passes all the tests
  • this also turns on warnAboutDeprecatedLifecycles for the test renderer. I think we missed doing so it previously. In a future PR, I'll remove the feature flag altogether.

Things to do in future PRs asap -

Screenshot:
image

@sizebot

This comment has been minimized.

Copy link

commented Jul 10, 2019

Details of bundled changes.

Comparing: 9f39590...9505272

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +0.2% 0.0% 629.08 KB 630.25 KB 138 KB 138.03 KB UMD_DEV
react-art.production.min.js 0.0% 0.0% 102.26 KB 102.26 KB 31.3 KB 31.3 KB UMD_PROD
react-art.development.js +0.2% 0.0% 559.94 KB 561.11 KB 120.63 KB 120.65 KB NODE_DEV
react-art.production.min.js 0.0% 0.0% 67.31 KB 67.31 KB 20.62 KB 20.62 KB NODE_PROD
ReactART-dev.js +0.2% -0.0% 572.96 KB 574.37 KB 120.21 KB 120.18 KB FB_WWW_DEV
ReactART-prod.js 0.0% -0.0% 218.46 KB 218.46 KB 37.08 KB 37.08 KB FB_WWW_PROD

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.profiling.min.js 0.0% 0.0% 115.01 KB 115.01 KB 36.26 KB 36.26 KB NODE_PROFILING
react-dom-server.browser.development.js +0.1% +0.1% 135.74 KB 135.84 KB 35.81 KB 35.86 KB UMD_DEV
ReactDOM-dev.js +0.2% -0.0% 915.52 KB 916.93 KB 203.49 KB 203.46 KB FB_WWW_DEV
react-dom-test-utils.development.js 0.0% 0.0% 57.96 KB 57.96 KB 15.91 KB 15.91 KB UMD_DEV
react-dom-unstable-fizz.browser.development.js 0.0% +0.1% 3.78 KB 3.78 KB 1.52 KB 1.52 KB UMD_DEV
react-dom-test-utils.production.min.js 0.0% 0.0% 10.95 KB 10.95 KB 4.01 KB 4.01 KB UMD_PROD
react-dom-unstable-fizz.browser.production.min.js 0.0% 🔺+0.3% 1.21 KB 1.21 KB 706 B 708 B UMD_PROD
react-dom-unstable-fizz.browser.development.js 0.0% +0.1% 3.61 KB 3.61 KB 1.48 KB 1.48 KB NODE_DEV
react-dom-test-utils.production.min.js 0.0% -0.0% 10.71 KB 10.71 KB 3.95 KB 3.95 KB NODE_PROD
react-dom-unstable-fizz.browser.production.min.js 0.0% 🔺+0.3% 1.05 KB 1.05 KB 637 B 639 B NODE_PROD
react-dom.development.js +0.1% 0.0% 892.87 KB 894.04 KB 203.11 KB 203.13 KB UMD_DEV
react-dom.profiling.min.js 0.0% 0.0% 114.73 KB 114.73 KB 36.87 KB 36.87 KB UMD_PROFILING
react-dom.development.js +0.1% 0.0% 887.16 KB 888.33 KB 201.6 KB 201.62 KB NODE_DEV
react-dom-server.node.development.js +0.1% +0.1% 133.8 KB 133.9 KB 35.43 KB 35.47 KB NODE_DEV
react-dom.production.min.js 0.0% 0.0% 111.36 KB 111.36 KB 35.32 KB 35.32 KB NODE_PROD
ReactDOM-profiling.js 0.0% 0.0% 377.65 KB 377.65 KB 69.39 KB 69.39 KB FB_WWW_PROFILING
react-dom-server.browser.development.js +0.1% +0.1% 131.88 KB 131.97 KB 34.89 KB 34.93 KB NODE_DEV
react-dom-unstable-native-dependencies.development.js 0.0% 0.0% 60.76 KB 60.76 KB 15.85 KB 15.85 KB UMD_DEV
react-dom-unstable-native-dependencies.production.min.js 0.0% 0.0% 10.74 KB 10.74 KB 3.68 KB 3.68 KB UMD_PROD
ReactDOMServer-dev.js 0.0% +0.1% 134.27 KB 134.32 KB 34.45 KB 34.49 KB FB_WWW_DEV
ReactDOMServer-prod.js 0.0% -0.0% 46.67 KB 46.67 KB 10.74 KB 10.74 KB FB_WWW_PROD
react-dom-unstable-native-dependencies.development.js 0.0% 0.0% 60.43 KB 60.43 KB 15.72 KB 15.72 KB NODE_DEV
react-dom-unstable-fizz.node.production.min.js 0.0% 🔺+0.1% 1.1 KB 1.1 KB 668 B 669 B NODE_PROD

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactTestRenderer-dev.js +0.2% -0.0% 582.88 KB 584.29 KB 122.54 KB 122.52 KB FB_WWW_DEV
react-test-renderer-shallow.development.js 0.0% 0.0% 35.6 KB 35.6 KB 9.41 KB 9.42 KB NODE_DEV
react-test-renderer-shallow.production.min.js 0.0% 0.0% 11.8 KB 11.8 KB 3.68 KB 3.68 KB NODE_PROD
react-test-renderer.development.js +0.2% 0.0% 572.29 KB 573.46 KB 123.31 KB 123.33 KB UMD_DEV
react-test-renderer.production.min.js 0.0% 0.0% 68.67 KB 68.67 KB 21.07 KB 21.07 KB UMD_PROD
react-test-renderer.development.js +0.2% 0.0% 567.83 KB 569 KB 122.17 KB 122.2 KB NODE_DEV
react-test-renderer.production.min.js 0.0% 0.0% 68.38 KB 68.38 KB 20.82 KB 20.82 KB NODE_PROD

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js +0.2% 0.0% 558.14 KB 559.32 KB 119.16 KB 119.18 KB NODE_DEV
react-reconciler-reflection.development.js 0.0% 0.0% 18.66 KB 18.66 KB 6.03 KB 6.03 KB NODE_DEV
react-reconciler-reflection.production.min.js 0.0% 🔺+0.1% 2.58 KB 2.58 KB 1.13 KB 1.13 KB NODE_PROD
react-reconciler-persistent.development.js +0.2% 0.0% 555.73 KB 556.9 KB 118.1 KB 118.12 KB NODE_DEV
react-reconciler-persistent.production.min.js 0.0% 0.0% 68.57 KB 68.57 KB 20.41 KB 20.41 KB NODE_PROD

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-profiling.js 0.0% 0.0% 280.67 KB 280.67 KB 48.36 KB 48.36 KB RN_OSS_PROFILING
ReactFabric-dev.js +0.2% -0.0% 721.52 KB 722.93 KB 153.49 KB 153.45 KB RN_FB_DEV
ReactNativeRenderer-dev.js +0.2% -0.0% 707.96 KB 709.36 KB 150.87 KB 150.84 KB RN_OSS_DEV
ReactNativeRenderer-dev.js +0.2% -0.0% 708.05 KB 709.46 KB 150.92 KB 150.89 KB RN_FB_DEV
ReactFabric-dev.js +0.2% -0.0% 721.42 KB 722.83 KB 153.44 KB 153.41 KB RN_OSS_DEV

Generated by 🚫 dangerJS

@bvaughn
Copy link
Contributor

left a comment

These lifecycles are actually deprecated and will be removed in the next major version. I think changing the message to "not recommended" is a mistake.

Edit for clarity I think it's okay to say

Using UNSAFE_componentWillMount in strict mode is not recommended

But I don't think it's good to say:

Using componentWillMount in strict mode is not recommended

I think our warning needs to be more than just "not recommended" because the idea is that it will stop working in a future version.

@threepointone

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

Good point, happy to change it

@threepointone

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

How do you feel about “...has been renamed to...”? I’m only trying to avoid the word ‘deprecated’.

@bvaughn

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

I am okay with "has been renamed to" in addition to advising against its usage.

@threepointone threepointone force-pushed the threepointone:lifecycle-warnings branch from fd7faef to c1d0e06 Jul 11, 2019

@threepointone

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

I updated the diff only to pass the build error, will continue tweaking the wording. Still not 100% sure yet tbh.

@threepointone threepointone force-pushed the threepointone:lifecycle-warnings branch 4 times, most recently from ec7b480 to e7127aa Jul 11, 2019

@threepointone threepointone requested a review from bvaughn Jul 11, 2019

@threepointone threepointone force-pushed the threepointone:lifecycle-warnings branch 2 times, most recently from 1c57951 to d41920b Jul 11, 2019

@threepointone threepointone force-pushed the threepointone:lifecycle-warnings branch 4 times, most recently from abe5f20 to e296fc4 Jul 12, 2019

@threepointone threepointone requested review from bvaughn and trueadm Jul 12, 2019

@trueadm
Copy link
Contributor

left a comment

The usage of the phrase "(preferred in most cases)" seems unnecessary and confusing to me. What cases? Why are they preferred in some cases but not others?

I think it's important we get this right, otherwise we're just going to cause pain for quite a large segment of users who are upgrading from an older version.

@threepointone

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2019

updated messages, screenshot.

@bvaughn
Copy link
Contributor

left a comment

I mostly just reviewed the screenshots for this pass. I still feel like the separate bullets are confusing and need to be changed, but I'm not willing to fight about it.

@threepointone threepointone force-pushed the threepointone:lifecycle-warnings branch from 7ba2169 to 66a8586 Jul 15, 2019

@threepointone

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2019

For (hopefully) the last time, updated the code and the screenshot. I'll let this sit for a couple of hours, and then merge it in.

unify deprecated/unsafe lifecycle warnings, pass tests
- redoes #15431 from scratch, taking on the feedback there
- unifies the messaging between "deprecated" and UNSAFE_ lifecycle messages. It reorganizes ReactStrictModeWarnings.js to capture and flush all the lifecycle warnings in one procedure each.
- matches the warning from ReactPartialRenderer to match the above change
- passes all the tests
- this also turns on `warnAboutDeprecatedLifecycles` for the test renderer. I think we missed doing so it previously. In a future PR, I'll remove the feature flag altogether.
- this DOES NOT do the same treatment for context warnings, I'll do that in another PR too

@threepointone threepointone force-pushed the threepointone:lifecycle-warnings branch from 66a8586 to 9505272 Jul 15, 2019

@threepointone threepointone merged commit d9b4c55 into facebook:master Jul 15, 2019

12 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: flow Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: lint_build Your tests passed on CircleCI!
Details
ci/circleci: process_artifacts Your tests passed on CircleCI!
Details
ci/circleci: setup Your tests passed on CircleCI!
Details
ci/circleci: test_build Your tests passed on CircleCI!
Details
ci/circleci: test_build_prod Your tests passed on CircleCI!
Details
ci/circleci: test_dom_fixtures Your tests passed on CircleCI!
Details
ci/circleci: test_source Your tests passed on CircleCI!
Details
ci/circleci: test_source_persistent Your tests passed on CircleCI!
Details
ci/circleci: test_source_prod Your tests passed on CircleCI!
Details

@threepointone threepointone deleted the threepointone:lifecycle-warnings branch Jul 16, 2019

@gaearon gaearon referenced this pull request Jul 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.