Skip to content

Fix usage of console.error to prevent transform #24188

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

Merged
merged 1 commit into from
Mar 29, 2022

Conversation

yashsriv
Copy link
Contributor

@yashsriv yashsriv commented Mar 29, 2022

Summary

We were suppressing the react-internals/warning-args lint rule for the call to console.error in defaultOnRecoverableError which was leading to a bug on dev builds. See below.

As far as I could tell, the lint rule exists because on dev builds, we replace all calls to console.error with this error
function
which expects a format string + args and nothing else. We were trying to pass in an Error object directly. After this commit's change, we will still be passing in an Error object but the console.error call won't be transformed.

How did you test this change?

Tested via

$ yarn flow dom
$ yarn linc

@yashsriv yashsriv marked this pull request as ready for review March 29, 2022 07:30
@gaearon
Copy link
Collaborator

gaearon commented Mar 29, 2022

I think mixed is correct because there’s no guarantee it’s an Error that’s being thrown. People can throw anything, in practice.

I also don’t think we want to add a prefix to the message. Instead I suggest to replace console.error with console['error'] to make it skip the transform. We do that in a few other places too. Then it will use the browser function directly which accepts anything.

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

See above

We were suppressing the `react-internals/warning-args` lint rule
for the call to `console.error` in `defaultOnRecoverableError`.

As far as I could tell, the lint rule exists because on dev builds,
we replace all calls to `console.error` with [this error
function](https://github.com/facebook/react/blob/main/packages/shared/consoleWithStackDev.js#L31-L37)
which expects a format string + args and nothing else. We were trying
to pass in an `Error` object directly. After this commit's change,
we will still be passing an `Error` but the transform won't occur.
@yashsriv yashsriv force-pushed the fix-console-error-usage branch from 0ccac09 to ca25332 Compare March 29, 2022 09:33
@yashsriv yashsriv changed the title Fix typing of onRecoverableError and fix console.error Fix usage of console.error to prevent transform Mar 29, 2022
@yashsriv
Copy link
Contributor Author

yashsriv commented Mar 29, 2022

I also don’t think we want to add a prefix to the message. Instead I suggest to replace console.error with console['error'] to make it skip the transform. We do that in a few other places too. Then it will use the browser function directly which accepts anything.

Sorry..,. saw your Workplace comment after sending this PR. Updated to do what you have suggested.

I think mixed is correct because there’s no guarantee it’s an Error that’s being thrown. People can throw anything, in practice.

That change was unrelated anyways to the issue at hand but now I'm interested.... isn't this onRecoverableError called only via react internal code.... wouldn't you be able to ensure that its always an Error (especially with the flow typing enabeld)

@yashsriv yashsriv requested a review from gaearon March 29, 2022 09:38
@gaearon
Copy link
Collaborator

gaearon commented Mar 29, 2022

isn't this onRecoverableError called only via react internal code.... wouldn't you be able to ensure that its always an Error (especially with the flow typing enabed)

No because the error may come from the user code (we just caught it) so we can’t know its type.

@gaearon gaearon merged commit fe6e074 into facebook:main Mar 29, 2022
@yashsriv yashsriv deleted the fix-console-error-usage branch March 29, 2022 09:57
rickhanlonii pushed a commit that referenced this pull request Apr 13, 2022
We were suppressing the `react-internals/warning-args` lint rule
for the call to `console.error` in `defaultOnRecoverableError`.

As far as I could tell, the lint rule exists because on dev builds,
we replace all calls to `console.error` with [this error
function](https://github.com/facebook/react/blob/main/packages/shared/consoleWithStackDev.js#L31-L37)
which expects a format string + args and nothing else. We were trying
to pass in an `Error` object directly. After this commit's change,
we will still be passing an `Error` but the transform won't occur.
rickhanlonii pushed a commit that referenced this pull request Apr 14, 2022
We were suppressing the `react-internals/warning-args` lint rule
for the call to `console.error` in `defaultOnRecoverableError`.

As far as I could tell, the lint rule exists because on dev builds,
we replace all calls to `console.error` with [this error
function](https://github.com/facebook/react/blob/main/packages/shared/consoleWithStackDev.js#L31-L37)
which expects a format string + args and nothing else. We were trying
to pass in an `Error` object directly. After this commit's change,
we will still be passing an `Error` but the transform won't occur.
rickhanlonii pushed a commit that referenced this pull request Apr 14, 2022
We were suppressing the `react-internals/warning-args` lint rule
for the call to `console.error` in `defaultOnRecoverableError`.

As far as I could tell, the lint rule exists because on dev builds,
we replace all calls to `console.error` with [this error
function](https://github.com/facebook/react/blob/main/packages/shared/consoleWithStackDev.js#L31-L37)
which expects a format string + args and nothing else. We were trying
to pass in an `Error` object directly. After this commit's change,
we will still be passing an `Error` but the transform won't occur.
rickhanlonii pushed a commit that referenced this pull request Apr 14, 2022
We were suppressing the `react-internals/warning-args` lint rule
for the call to `console.error` in `defaultOnRecoverableError`.

As far as I could tell, the lint rule exists because on dev builds,
we replace all calls to `console.error` with [this error
function](https://github.com/facebook/react/blob/main/packages/shared/consoleWithStackDev.js#L31-L37)
which expects a format string + args and nothing else. We were trying
to pass in an `Error` object directly. After this commit's change,
we will still be passing an `Error` but the transform won't occur.
rickhanlonii pushed a commit that referenced this pull request Apr 14, 2022
We were suppressing the `react-internals/warning-args` lint rule
for the call to `console.error` in `defaultOnRecoverableError`.

As far as I could tell, the lint rule exists because on dev builds,
we replace all calls to `console.error` with [this error
function](https://github.com/facebook/react/blob/main/packages/shared/consoleWithStackDev.js#L31-L37)
which expects a format string + args and nothing else. We were trying
to pass in an `Error` object directly. After this commit's change,
we will still be passing an `Error` but the transform won't occur.
zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Apr 15, 2022
We were suppressing the `react-internals/warning-args` lint rule
for the call to `console.error` in `defaultOnRecoverableError`.

As far as I could tell, the lint rule exists because on dev builds,
we replace all calls to `console.error` with [this error
function](https://github.com/facebook/react/blob/main/packages/shared/consoleWithStackDev.js#L31-L37)
which expects a format string + args and nothing else. We were trying
to pass in an `Error` object directly. After this commit's change,
we will still be passing an `Error` but the transform won't occur.
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