-
Notifications
You must be signed in to change notification settings - Fork 47k
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
Deprecation warnings use console.warn instead of console.error #9395
Comments
This means we will no see the stack trace? |
Sorry for what turned out to be a longer post than I expected! I've been thinking about warnings and stuff like this for some time it seems! If I recall correctly, a while ago React used to use Currently, there is currently no public API for users to hook into warnings that react generates. This sort of thing is useful for a number of reasons - but one of them is giving users control over what to do with warnings generated during test suites. Converting test suite warnings into errors is pretty widespread:
The suggested core change is to change some messages to a different type, because the core team (quite reasonably) thinks those messages are of a different category to the others and shouldn't fail a build. Some number of users have upgraded the warnings react produces by default to be even stronger, because they think they should fail the build. I think using All API changes have a cost, and because of the aforementioned widespread hooking into console.error, I think this change is unfortunately an API change. It's unclear to me exactly what the impact to the ecosystem is. As there'll be an ecosystem impact as users decide how they want to handle the new types of warning in test suites and elsewhere, perhaps this is a good time to make a larger but arguably better and more future-proof change. One of the things mentioned in #5306 and a number of the issues which link to it is to have a pluggable errors/warnings API. Such an API would be able to classify each warning React produces by its cause, and allow the user to decide what action to take based on this. The default behaviour could match what is being suggested in the OP, but the extension point would provide more control. For example, I might want to configure my test suite to
At the moment the only way to do something like this is by matching strings, and the suggested change (I think) moves us further away from this rather than closer. |
Whatever the impact is it is likely lower than us adding new
FWIW matching strings is exactly what we do at FB and it worked fairly well. |
@montogeek Stack trace is available for |
I think it'll make less builds fail, for sure - but anyone who is monkey-patching console.error to cause failures probably wanted it to fail. Assuming they know this is happening they'll need to expand the monkey patching. Obviously this is the opposite case to newcomers being put off by a load of warnings caused by dependencies they don't control - I appreciate you have to balance priorities / importance of this stuff.
Right, but it seems pretty error prone - it's more manageable in the case of FB where you control both the output and the matching code, but it's not like text changes to these warning messages are guaranteed to get called out in release notes? It's clearly being used as an API, so pleeeeeeeease can we make it an explicit one 😄 |
What kind of API do you propose? Our previous attempt at building extension points has failed before we even exposed them because it was over-abstracted, introduced too much ceremony, and was slow. We need to be able to quickly add, change, and remove warnings, and having an explicit API for opting into or out of different kinds of warnings will likely slow us down. (And our release process is already slow and error prone, as you might have experienced.) I feel like the ability to whitelist or blacklist based on substring matches might be the best tradeoff so we’d like to explore that. I have doubts about more sophisticated techniques.
Well.. Internally we just added this script that prints all warning messages and run it on every sync to see the diff. Might as well commit that file to the repo? Then you would be able to see the changes between versions. |
At least, that would be nice to have access to those warnings via some constants. It would make the blacklist / whitelist easier. |
That sounds like a great idea.
Strawman to start some discussion: In order to make the change described in the OP, I think the warnings lib would need to take a param for error/warning, so perhaps building on that a new warning lib might expose a signature like: warning(condition: boolean, category: string, format: string, ...args); Category could be a free string or perhaps a flow enum. If the set of categories doesn't change very often, but the messages themselves do - that would be probably be fairly straightforward for a consumer to manage keeping up with. The consumer-facing API is a trickier prospect, but as a starting point for discussion how about something really specific: React.handleWarning(category: string, callback: function) Any category not mentioned would keep the default behaviour perhaps? Are there any guarantees about when warnings will be raised? Could a similar API to the error boundaries handleError be used? Clearly my suggestions aren't likely to be quick or easy to put into practice, but the current state of this problem and proposed solutions has existed for a few years. Is there something special about 15.6 which means a quick change needs to be dropped in? |
I just came here to propose exactly the same. if its a warning it should use the appropriate console.warn() api and not an error. People using unit tests that fail on console.errors() are doomed using this. |
This is great discussion - we definitely hear the need for a better API around warnings. This has come up before and it's great to have more examples and feedback documented here. I think we are open to adding a more complete way to configure warnings for future versions. To do this well will require more attention, and we are starting to figure out where it will fit on our roadmap. Once that happens we can open a separate issue for continuing the discussion. Tagging @spicyj and @tomocchino for visibility, since we were recently talking about improving React warnings. |
**RFC:** Pushing this for feedback while I fix all the failing tests. **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
**RFC:** Pushing this for feedback while I fix all the failing tests. **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
**RFC:** Pushing this for feedback while I fix all the failing tests. **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
**RFC:** Pushing this for feedback while I fix all the failing tests. **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
**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
**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
**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
**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
* Initial regeneration of results.json **what is the change?:** We ran `yarn build` and updated the perf. stats record. **why make this change?:** Some commits have landed without updating this. By getting an initial update, I can run the build script again after my changes and see any size regressions. * 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 - #9395 **test plan:** `npm run test` and unit tests for the new modules and manual testing (WIP) **issue:** #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:** #9398 * Make state mutations an error, not low-pri warning **what is the change?:** Even though this is a "deprecation" warning, we still want to use 'console.error' for it. **why make this change?:** - It's not likely to come up now, hopefully, because this warning has been present for some time - This will cause real issues in production if ignored **test plan:** `yarn test` - we did fix one test which failed bc of this change **issue:** #9398 * Fix test of assigning to this.state that was only passing in fiber **what is the change?:** updated a unit test for assigning directly to state; it once again raises an error and not a warning. **why make this change?:** So that tests pass **test plan:** REACT_DOM_JEST_USE_FIBER=1 yarn run test **issue:** * Update results.json
* 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
* Downgrade deprecation warnings from errors to warnings (#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 - #9395 **test plan:** `npm run test` and unit tests for the new modules and manual testing (WIP) **issue:** #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:** #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
* 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
We did this in 15.6. |
Historically, React has logged warnings to
console.error
(rather thanconsole.warn
). I believe this has been done for a variety of reasons:console.warn
calls.Deprecation warnings are intended to provide users a heads-up on features that will be changing/breaking in the next major release. Up until now we have also logged these warnings to
console.error
. This makes them more noticeable but comes at the cost of:console.error
calls as fatal.After some discussion the React team has decided to replace
console.error
calls withconsole.warn
for deprecation warnings only. We intend to release this change in the upcoming 15.6 release.The text was updated successfully, but these errors were encountered: