Remove method name prefix from warnings and errors #28432
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
facebook-github-bot
added
CLA Signed
React Core Team
|
I also don't think we should add |
|
I'm not opposed to this change, but I wanted to provide some historical context. This was a practice adopted from some early Facebook codebases when stack traces, symbolication, and production reporting were still a total mess. Providing these helped give us some semblance of diagnostic similar to the top stack frame in modern tooling.
Also not opposed, just want to make you aware of this dependent: |
sebmarkbage
force-pushed
the
rmerrorprefixes
branch
from
708b296
to
18ff436
Compare
DevTools itself has to do the same thing but the "correct" (meaning consistent) way is this check: react/packages/react-devtools-shared/src/backend/console.js Lines 202 to 203 in 8fb0233 Maybe we can update LogBox to do the same check? |
sebmarkbage
force-pushed
the
rmerrorprefixes
branch
2 times, most recently
from
320cc9c
to
af31c9b
Compare
gnoff
approved these changes
Feb 23, 2024
sebmarkbage
commented
Feb 23, 2024
| "121": "performUpdateIfNecessary: Unexpected batch number (current %s, pending %s)", | ||
| "122": "%s(...): Expected the last optional `callback` argument to be a function. Instead received: %s.", |
There was a problem hiding this comment.
Since this shifts where the second argument is, I'm going to give this a new error code. Otherwise args from old links would end up showing the argument in the wrong place on the docs page.
sebmarkbage
commented
Feb 23, 2024
scripts/error-codes/codes.json
Outdated
| "279": "Trying to release an event instance into a pool of a different type.", | ||
| "280": "setRestoreImplementation() needs to be called to handle a target for controlled events. This error is likely caused by a bug in React. Please file an issue.", | ||
| "281": "Finished root should have a work-in-progress. This error is likely caused by a bug in React. Please file an issue.", | ||
| "282": "If the root does not have an updateQueue, we should have already bailed out. This error is likely caused by a bug in React. Please file an issue.", | ||
| "283": "Element type is invalid. Received a promise that resolves to: %s. Promise elements must resolve to a class or function.", | ||
| "284": "Expected ref to be a function, a string, an object returned by React.createRef(), or null.", | ||
| "285": "The root failed to unmount after an error. This is likely a bug in React. Please file an issue.", | ||
| "286": "%s(...): the first argument must be a React class instance. Instead received: %s.", |
sebmarkbage
force-pushed
the
rmerrorprefixes
branch
2 times, most recently
from
73e340c
to
97b4970
Compare
This pattern is a petpeeve of mine. I don't consider this best practice and so most don't have these prefixes. Very inconsistent. At best this is useless and noisey that you have to parse because the information is also in the stack trace. At worse these are misleading because they'rehighlighting something internal (like validateDOMNesting) which even suggests an internal bug. Even the ones public to React aren't necessarily what you called because you might be calling a wrapper around it. That would be properly reflected in a stack trace - which can also properly ignore list so that the first stack you see is your callsite, Which might be like render() in react-testing-library rather than createRoot() for example.
sebmarkbage
force-pushed
the
rmerrorprefixes
branch
from
97b4970
to
8515602
Compare
github-actions bot
pushed a commit
that referenced
this pull request
Feb 23, 2024
This pattern is a petpeeve of mine. I don't consider this best practice and so most don't have these prefixes. Very inconsistent. At best this is useless and noisey that you have to parse because the information is also in the stack trace. At worse these are misleading because they're highlighting something internal (like validateDOMNesting) which even suggests an internal bug. Even the ones public to React aren't necessarily what you called because you might be calling a wrapper around it. That would be properly reflected in a stack trace - which can also properly ignore list so that the first stack you see is your callsite, Which might be like `render()` in react-testing-library rather than `createRoot()` for example. DiffTrain build for [d579e77](d579e77)
|
@sebmarkbage the WarningFilter depends on that |
huozhi
added a commit
to vercel/next.js
that referenced
this pull request
Feb 26, 2024
### React upstream changes - facebook/react#28438 - facebook/react#28436 - facebook/react#25954 - facebook/react#28434 - facebook/react#28433 - facebook/react#28432 - facebook/react#28415 - facebook/react#27903 - facebook/react#28430 - facebook/react#28424 - facebook/react#28400 - facebook/react#28422 - facebook/react#28423 - facebook/react#28412 - facebook/react#28418 - facebook/react#28421 - facebook/react#28417 - facebook/react#28399 - facebook/react#28408 - facebook/react#28350 - facebook/react#28387 - facebook/react#28403 - facebook/react#28384 - facebook/react#28409 - facebook/react#28398 - facebook/react#28405 - facebook/react#28328 - facebook/react#28402 - facebook/react#28386 - facebook/react#28388 - facebook/react#28379 - facebook/react#28383 - facebook/react#28390 - facebook/react#28389 - facebook/react#28382 - facebook/react#28348 Closes NEXT-2600
EdisonVan
pushed a commit
to EdisonVan/react
that referenced
this pull request
Apr 15, 2024
This pattern is a petpeeve of mine. I don't consider this best practice and so most don't have these prefixes. Very inconsistent. At best this is useless and noisey that you have to parse because the information is also in the stack trace. At worse these are misleading because they're highlighting something internal (like validateDOMNesting) which even suggests an internal bug. Even the ones public to React aren't necessarily what you called because you might be calling a wrapper around it. That would be properly reflected in a stack trace - which can also properly ignore list so that the first stack you see is your callsite, Which might be like `render()` in react-testing-library rather than `createRoot()` for example.
bigfootjon
pushed a commit
that referenced
this pull request
Apr 18, 2024
This pattern is a petpeeve of mine. I don't consider this best practice and so most don't have these prefixes. Very inconsistent. At best this is useless and noisey that you have to parse because the information is also in the stack trace. At worse these are misleading because they're highlighting something internal (like validateDOMNesting) which even suggests an internal bug. Even the ones public to React aren't necessarily what you called because you might be calling a wrapper around it. That would be properly reflected in a stack trace - which can also properly ignore list so that the first stack you see is your callsite, Which might be like `render()` in react-testing-library rather than `createRoot()` for example. DiffTrain build for commit d579e77.
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.
This pattern is a petpeeve of mine. I don't consider this best practice and so most don't have these prefixes. Very inconsistent.
At best this is useless and noisey that you have to parse because the information is also in the stack trace.
At worse these are misleading because they're highlighting something internal (like validateDOMNesting) which even suggests an internal bug. Even the ones public to React aren't necessarily what you called because you might be calling a wrapper around it.
That would be properly reflected in a stack trace - which can also properly ignore list so that the first stack you see is your callsite,
Which might be like
render()in react-testing-library rather thancreateRoot()for example.