Improve low priority warning #9754

flarnie · 2017-05-23T18:41:42Z

Thanks to @spicyj for suggesting this in code review of #9650

what is the change?:
This change makes 'lowPriorityWarning' an exact copy of 'warning.js' from
https://github.com/facebook/fbjs/blob/e66ba20ad5be433eb54423f2b097d829324d9de6/packages/fbjs/src/__forks__/warning.js
where before we had skipped some checks from that module.

Adds an error which we catch, in order to let people find the error and resulting stack trace when using devtools with 'pause on caught errors' checked.
Adds check that 'format' argument is passed

why make this change?:

To maintain a closer fork to 'warning.js'
To allow easier debugging using 'pause on caught errors'
To validate inputs to 'lowPriorityWarning'

test plan:
yarn test

issue:
#9398

**what is the change?:** This change makes 'lowPriorityWarning' an exact copy of 'warning.js' from https://github.com/facebook/fbjs/blob/e66ba20ad5be433eb54423f2b097d829324d9de6/packages/fbjs/src/__forks__/warning.js where before we had skipped some checks from that module. - Adds an error which we catch, in order to let people find the error and resulting stack trace when using devtools with 'pause on caught errors' checked. - Adds check that 'format' argument is passed **why make this change?:** - To maintain a closer fork to 'warning.js' - To allow easier debugging using 'pause on caught errors' - To validate inputs to 'lowPriorityWarning' **test plan:** `yarn test` **issue:**

flarnie · 2017-05-23T18:42:03Z

Note to self - this will also need to be cherry-picked onto 15.6-dev.

**what is the change?:** We print the logs from 'lowPriorityWarning' as well as 'warning' from the 'print-warnings' script. NOTE: This PR is branching off of facebook#9754 **why make this change?:** We want to use the same process of white/blacklisting warnings with 'lowPriorityWarning' that we do with 'warning'. **test plan:** This is not super easy to test unless we are doing a sync with FB afaik. I plan on running a sync in the next few days, or next week at latest, for the sake of not landing big things on a Friday. That will be the actual test of this. **issue:** facebook#9398

flarnie · 2017-05-23T22:43:19Z

PS to self - rebase #9756 after landing this.

flarnie · 2017-05-24T14:32:38Z

bvaughn

I realize it's probably too late to change this, given all the code that uses fb warning but...this makes me wonder, again, why warning doesn't just use console.warn in the first place.

bvaughn · 2017-05-24T14:44:52Z

scripts/rollup/results.json

-      "size": 496986,
-      "gzip": 118763
+      "size": 497667,
+      "gzip": 118999


Should we be manually updating bundle sizes like this? I feel like I'm still never sure.

I think every time. This came up in code review before, on #9420 and @trueadm said:

For now, whenever you update something that will be merged into master (it's always good to merge master into your branch and run this after). It gives us all some indications on possible mistakes in terms of bundle size regressions. We'll improve how this works in the coming weeks too.

We could probably update the 'contributing' docs to mention this if they don't already. I don't think most people are.

Or automate it - even better. :)

I don't like this idea. I think it is asking for troubles when it comes to merge conflicts.

IMO we should be updating this file for when a PR is ready, as it updates master so it has the latest sizes – which is great for indicating when we've maybe let DEV code slip in unexpectedly.

I'm not sure how we handle the issues with merge conflicts though :/

I just feel like that will cause a lot of problems. We're basically guaranteeing that every merged PR will conflict every other PR, requiring master-merges or rebases, etc. It seems like, in the extreme, we'll have total gridlock. And unless we always rebase and rebuild before merging, we can't trust the file sizes in the results file.

**what is the change?:** We print the logs from 'lowPriorityWarning' as well as 'warning' from the 'print-warnings' script. NOTE: This PR is branching off of facebook#9754 **why make this change?:** We want to use the same process of white/blacklisting warnings with 'lowPriorityWarning' that we do with 'warning'. **test plan:** This is not super easy to test unless we are doing a sync with FB afaik. I plan on running a sync in the next few days, or next week at latest, for the sake of not landing big things on a Friday. That will be the actual test of this. **issue:** facebook#9398

* Add back caught error and other checks to 'lowPriorityWarning' **what is the change?:** This change makes 'lowPriorityWarning' an exact copy of 'warning.js' from https://github.com/facebook/fbjs/blob/e66ba20ad5be433eb54423f2b097d829324d9de6/packages/fbjs/src/__forks__/warning.js where before we had skipped some checks from that module. - Adds an error which we catch, in order to let people find the error and resulting stack trace when using devtools with 'pause on caught errors' checked. - Adds check that 'format' argument is passed **why make this change?:** - To maintain a closer fork to 'warning.js' - To allow easier debugging using 'pause on caught errors' - To validate inputs to 'lowPriorityWarning' **test plan:** `yarn test`

* Add back caught error and other checks to 'lowPriorityWarning' **what is the change?:** This change makes 'lowPriorityWarning' an exact copy of 'warning.js' from https://github.com/facebook/fbjs/blob/e66ba20ad5be433eb54423f2b097d829324d9de6/packages/fbjs/src/__forks__/warning.js where before we had skipped some checks from that module. - Adds an error which we catch, in order to let people find the error and resulting stack trace when using devtools with 'pause on caught errors' checked. - Adds check that 'format' argument is passed **why make this change?:** - To maintain a closer fork to 'warning.js' - To allow easier debugging using 'pause on caught errors' - To validate inputs to 'lowPriorityWarning' **test plan:** `yarn test` **issue:** * Update 'print-warnings' script to include 'lowPriorityWarning' output **what is the change?:** We print the logs from 'lowPriorityWarning' as well as 'warning' from the 'print-warnings' script. NOTE: This PR is branching off of #9754 **why make this change?:** We want to use the same process of white/blacklisting warnings with 'lowPriorityWarning' that we do with 'warning'. **test plan:** This is not super easy to test unless we are doing a sync with FB afaik. I plan on running a sync in the next few days, or next week at latest, for the sake of not landing big things on a Friday. That will be the actual test of this. **issue:** #9398

* Add back caught error and other checks to 'lowPriorityWarning' **what is the change?:** This change makes 'lowPriorityWarning' an exact copy of 'warning.js' from https://github.com/facebook/fbjs/blob/e66ba20ad5be433eb54423f2b097d829324d9de6/packages/fbjs/src/__forks__/warning.js where before we had skipped some checks from that module. - Adds an error which we catch, in order to let people find the error and resulting stack trace when using devtools with 'pause on caught errors' checked. - Adds check that 'format' argument is passed **why make this change?:** - To maintain a closer fork to 'warning.js' - To allow easier debugging using 'pause on caught errors' - To validate inputs to 'lowPriorityWarning' **test plan:** `yarn test`

flarnie added 2 commits May 23, 2017 11:34

Update results.json

51ef279

flarnie requested a review from sophiebits May 23, 2017 18:41

facebook-github-bot added the CLA Signed label May 23, 2017

flarnie mentioned this pull request May 23, 2017

React 15.6 Umbrella #9398

Closed

49 tasks

flarnie mentioned this pull request May 23, 2017

Update print warning script for low priority warning #9756

Merged

flarnie requested a review from gaearon May 23, 2017 20:45

Run prettier

e65e7ce

flarnie requested review from bvaughn and acdlite May 24, 2017 14:32

bvaughn approved these changes May 24, 2017

View reviewed changes

flarnie merged commit 6ac91d2 into facebook:master May 24, 2017

flarnie deleted the improveLowPriorityWarning branch May 25, 2018 17:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve low priority warning #9754

Improve low priority warning #9754

flarnie commented May 23, 2017

flarnie commented May 23, 2017

flarnie commented May 23, 2017

flarnie commented May 24, 2017

bvaughn left a comment

bvaughn May 24, 2017

flarnie May 24, 2017

flarnie May 24, 2017

flarnie May 24, 2017

bvaughn May 24, 2017

trueadm May 24, 2017 •

edited

Loading

bvaughn May 24, 2017 •

edited

Loading

Improve low priority warning #9754

Improve low priority warning #9754

Conversation

flarnie commented May 23, 2017

flarnie commented May 23, 2017

flarnie commented May 23, 2017

flarnie commented May 24, 2017

bvaughn left a comment

Choose a reason for hiding this comment

bvaughn May 24, 2017

Choose a reason for hiding this comment

flarnie May 24, 2017

Choose a reason for hiding this comment

flarnie May 24, 2017

Choose a reason for hiding this comment

flarnie May 24, 2017

Choose a reason for hiding this comment

bvaughn May 24, 2017

Choose a reason for hiding this comment

trueadm May 24, 2017 • edited Loading

Choose a reason for hiding this comment

bvaughn May 24, 2017 • edited Loading

Choose a reason for hiding this comment

trueadm May 24, 2017 •

edited

Loading

bvaughn May 24, 2017 •

edited

Loading