ReactNativeFiberErrorDialog mutates error message #10045
Conversation
Without this property, all fiber errors get lumped together by Facebook's error tooling
sebmarkbage
left a comment
sebmarkbage
left a comment
There was a problem hiding this comment.
We should rethink this error cloning strategy. new errorType(errorMessage) is not a valid constructor argument for all types of errors.
Why can't we just mutate the existing error again?
|
|
||
| const summary = message ? `${name}: ${message}` : name; | ||
|
|
||
| errorMessage = `${summary}\n\nThis error is located at:${componentStack}`; |
There was a problem hiding this comment.
Could we just do error.message = errorMessage instead of cloning?
There was a problem hiding this comment.
So far as I know (based on very limited testing) that would work.
There was a problem hiding this comment.
Actually I think I was creating a new object to handle the case of a string or null being thrown (instead of an Error) but I can just change the code to only create a new Error in that case.
I don't recall if we had a specifically reason, other than generally wanting to avoid mutating parameters. |
|
|
||
| const newError = new errorType(errorMessage); | ||
| (newError: any).framesToPop = framesToPop; | ||
| newError.stack = errorStack; |
There was a problem hiding this comment.
This is not fully equivalent because debug tooling and such can read the internal slot of this instead of the property name.
| errorType = Error; | ||
| } | ||
|
|
||
| const newError = new errorType(errorMessage); |
There was a problem hiding this comment.
Like mention above, this constructor may have different signatures than accepting a message for some error types.
bvaughn
changed the title
|
Edit: It's showing up now. |
It also better handles string error types.
bvaughn
force-pushed
the
frames-to-pop
branch
2 times, most recently
from
2612d57 to
93d6ee5
Compare
|
"This is an actual error" lol |
|
Yeah, I'm not very creative when running through test cases 😛 |
|
Okay I'm going to assume this is good to go. Merging. |
|
|
||
| // Typically Errors are thrown but eg strings or null can be thrown as well. | ||
| if (error && typeof error === 'object') { | ||
| if (error instanceof Error) { |
There was a problem hiding this comment.
It might be an error from a different realm but in RN that's unusual or not available at all so this is probably fine. Besides this isn't critical if it gives a false negative.
| errorStack = error.stack; | ||
| errorType = error.constructor; | ||
| errorToHandle = error; | ||
| errorToHandle.message = `${summary}\n\nThis error is located at:${componentStack}`; |
There was a problem hiding this comment.
If this is a frozen object (or non-writable) this will throw. Leaving the error unhandled.
We could use Object.defineProperty(errorToHandle, 'message', { value: ${summary}\n\nThis error is located at:${componentStack} }) but even that might throw for a non-configurable property.
Maybe we should just wrap this in try/catch?
4 participants
The primary motivation for this diff is to fix an issue with the
framesToPopproperty (added byinvariantand others) not being copied over to the "cloned"Errorobject. This caused problems with Facebook's automated error tooling.After some discussion with Seb, we decided to just mutate the incoming
Errorrather than cloning. I also improved the error messaging for string-type errors on the way (as shown below in the screenshots).