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

Cherrypick commit to downgrade deprecations to warnings #9753

Conversation

flarnie
Copy link
Contributor

@flarnie flarnie commented May 23, 2017

This landed on master, and we want to pull it into 15.6. I tweaked a couple of things to make it work on this branch.

flarnie and others added 2 commits May 23, 2017 09:59
* Downgrade deprecation warnings from errors to warnings

**what is the change?:**
Swapping out `warning` module for a fork that uses `console.warn`.
It looks like we were using the `warning` module for deprecation notices, *but* there is also a 'deprecated' module designed specifically for deprecation notices.

However, we could not find any place that it was currently used.

Since React's build process is not 100% clear to me, I assume it could still be used somewhere by something and just updated it along with other deprecation notices.

We might consider a follow-up diff that does some clean up here;
 - remove 'deprecated' module if it's unused, OR
 - use 'deprecated' module for all our current deprecation warnings

**why make this change?:**
- We have had complaints about noisy warnings, in particular after introducing new deprecations
- They potentially cause CI failures
- Deprecations are not really time-sensitive, can ship without breaking your app, etc.

For more context - facebook#9395

**test plan:**
`npm run test`
and unit tests for the new modules
and manual testing (WIP)

**issue:**
facebook#9395

* Add 'lowPriorityWarning' to ReactExternals

**what is the change?:**
We won't bundle 'lowPriorityWarning' with the rest of React when building for Facebook.
NOTE: A parallel commit will introduce an internal implementation of 'lowPriorityWarning' in Facebook's codebase, to compensate. Will post a comment with the diff number once that is up.

**why make this change?:**
So that the sync between github and Facebook can go more smoothly!

**test plan:**
We will see when I run the sync! But this is a reasonable first step imo.

**issue:**
facebook#9398
**what is the change?:**
- adds 'lowPriorityWarning' for deprecation of '__spread' and 'createMixin'
- tweaks test to check for 'warn' and not 'error'

**why make this change?:**
Both these issues were introduced by merge conflict resolution when cherry-picking this change from master onto 15.6.

**test plan:**
`yarn test`

**issue:**
@flarnie flarnie added this to the 15.6 milestone May 23, 2017
@flarnie flarnie mentioned this pull request May 23, 2017
49 tasks
**what is the change?:**
Fixes 'warning' to be required from 'warning'

**why make this change?:**
It was causing the browserify build to crash, because we don't expect to have a path to 'warning'.

**test plan:**
CI
@flarnie flarnie requested a review from bvaughn May 23, 2017 20:57
Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

Love this change

@flarnie
Copy link
Contributor Author

flarnie commented May 23, 2017

:D Love that you were awake and reviewed this @bvaughn
thanks_pikachu

@flarnie flarnie merged commit 413c7b5 into facebook:15.6-dev May 23, 2017
flarnie added a commit to flarnie/react that referenced this pull request Jun 7, 2017
* Downgrade deprecation warnings from errors to warnings (facebook#9650)

* Downgrade deprecation warnings from errors to warnings

**what is the change?:**
Swapping out `warning` module for a fork that uses `console.warn`.
It looks like we were using the `warning` module for deprecation notices, *but* there is also a 'deprecated' module designed specifically for deprecation notices.

However, we could not find any place that it was currently used.

Since React's build process is not 100% clear to me, I assume it could still be used somewhere by something and just updated it along with other deprecation notices.

We might consider a follow-up diff that does some clean up here;
 - remove 'deprecated' module if it's unused, OR
 - use 'deprecated' module for all our current deprecation warnings

**why make this change?:**
- We have had complaints about noisy warnings, in particular after introducing new deprecations
- They potentially cause CI failures
- Deprecations are not really time-sensitive, can ship without breaking your app, etc.

For more context - facebook#9395

**test plan:**
`npm run test`
and unit tests for the new modules
and manual testing (WIP)

**issue:**
facebook#9395

* Add 'lowPriorityWarning' to ReactExternals

**what is the change?:**
We won't bundle 'lowPriorityWarning' with the rest of React when building for Facebook.
NOTE: A parallel commit will introduce an internal implementation of 'lowPriorityWarning' in Facebook's codebase, to compensate. Will post a comment with the diff number once that is up.

**why make this change?:**
So that the sync between github and Facebook can go more smoothly!

**test plan:**
We will see when I run the sync! But this is a reasonable first step imo.

**issue:**
facebook#9398

* Tweaks to get tests passing after cherry-picking PR#9650

**what is the change?:**
- adds 'lowPriorityWarning' for deprecation of '__spread' and 'createMixin'
- tweaks test to check for 'warn' and not 'error'

**why make this change?:**
Both these issues were introduced by merge conflict resolution when cherry-picking this change from master onto 15.6.

**test plan:**
`yarn test`

**issue:**

* Fix mis-written 'require' for 'warning' module

**what is the change?:**
Fixes 'warning' to be required from 'warning'

**why make this change?:**
It was causing the browserify build to crash, because we don't expect to have a path to 'warning'.

**test plan:**
CI
@flarnie flarnie deleted the cherrypickCommitToDowngradeDeprecationsToWarnings branch May 25, 2018 17:50
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.

3 participants