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

Update print warning script for low priority warning #9756

Merged

Conversation

flarnie
Copy link
Contributor

@flarnie flarnie commented May 23, 2017

NOTE: This PR was made on a branch that goes off of #9754 and I'll rebase it once that lands.

So when reviewing just look at the change in scripts/print-warnings/print-warnings.js.

 **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 https:/facebook/react/pull/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:**
https:/facebook/react/issues/9398

**what is the change?:**
This change makes 'lowPriorityWarning' an exact copy of 'warning.js' from
https:/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:**
**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 flarnie force-pushed the updatePrintWarningScriptForLowPriorityWarning branch from a2eca37 to 9e54672 Compare May 24, 2017 15:23
@flarnie flarnie requested a review from bvaughn May 25, 2017 12:58
@flarnie
Copy link
Contributor Author

flarnie commented May 25, 2017

boop
boop

@flarnie flarnie merged commit e6af158 into facebook:master May 26, 2017
@flarnie flarnie deleted the updatePrintWarningScriptForLowPriorityWarning branch May 25, 2018 17:51
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