Skip to content
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

Disable prepareStackTrace while we're generating stacks #18708

Merged
merged 1 commit into from Apr 23, 2020

Conversation

sebmarkbage
Copy link
Collaborator

This could be used to do custom formatting of the stack trace in a way that isn't compatible with how we use it. So we disable it while we use it.

In theory we could call this ourselves with the result of our stack. It would be a lot of extra production code though. My personal opinion is that this should always be done server side instead of on the client. This is not infrastructure that is worth its code size.

We could expose a custom parser that converts it and passes it through prepareStackTrace as structured data. That way it's external and doesn't have to be built-in to React.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Apr 23, 2020
@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 23, 2020

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 8d56927:

Sandbox Source
wonderful-feistel-nhnqx Configuration

@sizebot
Copy link

sizebot commented Apr 23, 2020

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 8d56927

@sizebot
Copy link

sizebot commented Apr 23, 2020

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against 8d56927

@sebmarkbage
Copy link
Collaborator Author

@bvaughn Should we port this to DevTools too?

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.

Ok but I think DEV condition is mismatching

@@ -182,6 +185,7 @@ export function describeNativeComponentFrame(
reentry = false;
if (__DEV__) {
ReactCurrentDispatcher.current = previousDispatcher;
Error.prepareStackTrace = previousPrepareStackTrace;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you mean to put this outside of DEV?

@bvaughn
Copy link
Contributor

bvaughn commented Apr 23, 2020

@bvaughn Should we port this to DevTools too?

Yeah seems worth adding.

We could expose a custom parser that converts it and passes it through prepareStackTrace as structured data. That way it's external and doesn't have to be built-in to React.

I'm not sure I understand this suggestion? We don't have enough info in the string (and so a custom parser wouldn't have enough info) to fully reconstruct a CallSight object.

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

Looks fine except for the mismatched DEV conditional.

This could be used to do custom formatting of the stack trace in a way
that isn't compatible with how we use it. So we disable it while we use
it.

In theory we could call this ourselves with the result of our stack.
It would be a lot of extra production code though. My personal opinion
is that this should always be done server side instead of on the client.

We could expose a custom parser that converts it and passes it through
prepareStackTrace as structured data. That way it's external and doesn't
have to be built-in to React.
@sebmarkbage sebmarkbage merged commit a2fb84b into facebook:master Apr 23, 2020
trentm added a commit to elastic/apm-agent-nodejs that referenced this pull request Jun 11, 2021
The root cause was facebook/react#18708
temporarily doing `Error.prepareStackTrace = undefined` to get a stack
string. This wasn't handled correctly by error-callsites
(watson/error-callsites#3).

Fixes: #1980
trentm added a commit to elastic/apm-agent-nodejs that referenced this pull request Jun 14, 2021
…7+ (#2107)

The root cause was facebook/react#18708
temporarily doing `Error.prepareStackTrace = undefined` to get a stack
string. This wasn't handled correctly by error-callsites
(watson/error-callsites#3).

Fixes: #1980
dgieselaar pushed a commit to dgieselaar/apm-agent-nodejs that referenced this pull request Sep 10, 2021
…7+ (elastic#2107)

The root cause was facebook/react#18708
temporarily doing `Error.prepareStackTrace = undefined` to get a stack
string. This wasn't handled correctly by error-callsites
(watson/error-callsites#3).

Fixes: elastic#1980
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants