Use console directly instead of warning() modules #17599
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Closed
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 443ef71:
|
gaearon
requested review from
walaura,
sebmarkbage,
acdlite,
bvaughn and
trueadm
Details of bundled changes.Comparing: b6c423d...443ef71 react
react-dom
react-native-renderer
react-test-renderer
react-interactions
react-noop-renderer
react-is
react-art
react-cache
react-reconciler
create-subscription
ReactDOM: size: 0.0%, gzip: -0.1% React: size: 0.0%, gzip: -0.0% Size changes (experimental) |
Details of bundled changes.Comparing: b6c423d...443ef71 react
react-dom
react-interactions
react-noop-renderer
react-reconciler
react-is
react-art
react-test-renderer
react-cache
react-native-renderer
create-subscription
Size changes (stable) |
|
If one of our goals is to be more rigorous about adding warnings, I wonder if we could take the same approach we use for the error transform, where we maintain the list of error messages in the source, and only transform the ones that have a matching entry. We use a lint rule to enforce that each message has a matching entry. This makes adding/editing warnings slightly more annoying but it comes with some benefits:
The downside is that adding a warning means also updating the warning list. But adding some inertia there could be a feature, too. We've gotten feedback that we have too many warnings of varying severity and no way to opt out of them, so it could behoove us to start being more intentional about adding them. (And if we ever do provide a way for developers to opt-out of specific warnings, similar to the one used by www, we would want to track them.) |
bvaughn
approved these changes
Dec 13, 2019
|
@acdlite I’m down for this! Follow-up or here? |
|
@gaearon Follow up is fine, good to merge as-is |
|
I pushed another commit that makes the "gotchas" clear by failing the build when |
|
This PR broke DevTools tests. After this commit was merged, running |
NMinhNguyen
referenced
this pull request
in enzymejs/react-shallow-renderer
Jan 29, 2020
* Replace all warning/lowPriWarning with console calls * Replace console.warn/error with a custom wrapper at build time * Fail the build for console.error/warn() where we can't read the stack
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Picking up where #17568 and #17586 left.
Related to #16753.
This replaces four custom modules in the source with direct calls:
shared/warning->console.errorshared/warningWithoutStack->console.errorshared/lowPriorityWarning->console.warnshared/lowPriorityWarningWithoutStack->console.warnTo review, you want to look at the second commit because the first one is just a find-replace.
Here's a few points about the new system.
Not Removed in Prod
We're not doing anything special to remove
console.errororconsole.warncalls in prod anymore. If you keep them, they'll be there. We do have a linter plugin that tells you to wrap it in DEV. I audited existing cases where it fails, and added suppressions where it was intentional.Stack Is Injected Automatically
You don't need to choose whether to inject the stack. If you use
console.error, it will be injected automatically, as long as the package from which you're calling it has access to the React object. Alternatively, it will keep yourconsole.erroras is in the source (no stack). DevTools would likely pick it up though anyway.Note: in the future we might completely remove the stack injection and always rely on DevTools and similar mechanisms (e.g. RN integration) to inject it.
Escape Hatches
If you want to use the "raw"
console.errororconsole.warnfor whatever reason, you can do it like this:This will use the native
console.errorin DEV too. Practically, this means we won't try to append the stack, prepend theWarning:marker (which is currently relied on by RN), or go through the WWW whitelist. There's only a few places that we do this.Gotchas
We have two modules that don't have access to the isomorphic React package:
schedulerandreact-is. So we can't append the stacks. This meansconsole.errorcalls inside of these two packages won't get transformed even in DEV for real bundles —they'll stay asconsole.error.But in our tests, they would be transformed.(I changed this to fail the build.)We barely have any warnings there, and both are meant to be lightweight. So I think it's okay. The fact that they don't go through the WWW list can be confusing. But in longer term we want to get rid of the special
warningentry point altogether. In that world, we would have the list be a list of warnings to mute, and pass everything else through.To get there, we'll need to enforce that the React warning list matches WWW list 1:1. We'd need to get buy-in from other teams for that to work out. In the meantime, I don't expect we'll add a lot of new warnings to either
schedulerorreact-isanyway.If this gotcha ends up being a pain, we can always have a second "compile target" that exists solely to make these warnings go through the WWW list anyway.
Risks
Let's sync WWW carefully after this. We need to verify nothing broke and warnings still fire. I can do this.
Follow-ups
Out of scope of this PR, but after landing this I'm planning to rename
toWarnDevtotoErrorDev(because it's forconsole.error), andtoLowPriorityWarnDevtotoWarnDev(because it's forconsole.warn). Then I plan to audit the warnings and split them into these two levels according to whether they represent real issues or not.Additionally, I'll remove the cases where we detect presence of custom stack first before appending it. Instead, we'll set the current fiber.