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.

Sign up for GitHub

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

Jump to bottom

Align earlyjs c++ stack trace parsing with js #46894

Closed
wants to merge 2 commits into from
Closed

Align earlyjs c++ stack trace parsing with js #46894

wants to merge 2 commits into from

Conversation

RSNara
Copy link
Contributor

@RSNara RSNara commented Oct 8, 2024

Summary:
This diff re-implements js error stack trace parsing in c++.

Details:

I also migrated all their tests to c++:

Changelog: [Internal]

Reviewed By: javache, NickGerleman

Differential Revision: D63659013

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 8, 2024
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D63659013

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D63659013

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D63659013

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D63659013

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D63659013

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D63659013

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D63659013

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D63659013

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D63659013

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D63659013

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D63659013

RSNara added a commit to RSNara/react-native that referenced this pull request Oct 9, 2024
@RSNara @facebook-github-bot
Align earlyjs c++ stack trace parsing with js (facebook#46894)
2653774 
Summary:

This diff re-implements [js error stack trace parsing](https://github.com/facebook/react-native/blob/86cac6836502aaeb5c894bff6427e837c52c09e0/packages/react-native/Libraries/Core/Devtools/parseErrorStack.js#L41-L57) in c++.

Details:
- I migrated [stacktrace-parser](https://github.com/errwischt/stacktrace-parser/blob/ad379de5e5ac056012bbeb12923cf502aefe4710/src/stack-trace-parser.js#L7)
- I migrated [parseHermesStack.js](https://github.com/facebook/react-native/blob/86cac6836502aaeb5c894bff6427e837c52c09e0/packages/react-native/Libraries/Core/Devtools/parseHermesStack.js#L82)

I also migrated all their tests to c++:
- [stacktrace-parser tests](https://github.com/errwischt/stacktrace-parser/blob/ad379de5e5ac056012bbeb12923cf502aefe4710/test/stack-trace-parser.spec.js#L5)
- [parseHermesStack tests](https://github.com/facebook/react-native/blob/86cac6836502aaeb5c894bff6427e837c52c09e0/packages/react-native/Libraries/Core/Devtools/__tests__/parseHermesStack-test.js#L16)

Changelog: [Internal]

Reviewed By: javache, NickGerleman

Differential Revision: D63659013
RSNara added a commit to RSNara/react-native that referenced this pull request Oct 9, 2024
@RSNara @facebook-github-bot
[skip ci] Align earlyjs c++ stack trace parsing with js (facebook#46894)
8ebc817 
Summary:

This diff re-implements [js error stack trace parsing](https://github.com/facebook/react-native/blob/86cac6836502aaeb5c894bff6427e837c52c09e0/packages/react-native/Libraries/Core/Devtools/parseErrorStack.js#L41-L57) in c++.

Details:
- I migrated [stacktrace-parser](https://github.com/errwischt/stacktrace-parser/blob/ad379de5e5ac056012bbeb12923cf502aefe4710/src/stack-trace-parser.js#L7)
- I migrated [parseHermesStack.js](https://github.com/facebook/react-native/blob/86cac6836502aaeb5c894bff6427e837c52c09e0/packages/react-native/Libraries/Core/Devtools/parseHermesStack.js#L82)

I also migrated all their tests to c++:
- [stacktrace-parser tests](https://github.com/errwischt/stacktrace-parser/blob/ad379de5e5ac056012bbeb12923cf502aefe4710/test/stack-trace-parser.spec.js#L5)
- [parseHermesStack tests](https://github.com/facebook/react-native/blob/86cac6836502aaeb5c894bff6427e837c52c09e0/packages/react-native/Libraries/Core/Devtools/__tests__/parseHermesStack-test.js#L16)

Changelog: [Internal]

Reviewed By: javache, NickGerleman

Differential Revision: D63659013
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D63659013

RSNara added a commit to RSNara/react-native that referenced this pull request Oct 9, 2024
@RSNara @facebook-github-bot
[skip ci] Align earlyjs c++ stack trace parsing with js (facebook#46894)
a995e20 
Summary:

This diff re-implements [js error stack trace parsing](https://github.com/facebook/react-native/blob/86cac6836502aaeb5c894bff6427e837c52c09e0/packages/react-native/Libraries/Core/Devtools/parseErrorStack.js#L41-L57) in c++.

Details:
- I migrated [stacktrace-parser](https://github.com/errwischt/stacktrace-parser/blob/ad379de5e5ac056012bbeb12923cf502aefe4710/src/stack-trace-parser.js#L7)
- I migrated [parseHermesStack.js](https://github.com/facebook/react-native/blob/86cac6836502aaeb5c894bff6427e837c52c09e0/packages/react-native/Libraries/Core/Devtools/parseHermesStack.js#L82)

I also migrated all their tests to c++:
- [stacktrace-parser tests](https://github.com/errwischt/stacktrace-parser/blob/ad379de5e5ac056012bbeb12923cf502aefe4710/test/stack-trace-parser.spec.js#L5)
- [parseHermesStack tests](https://github.com/facebook/react-native/blob/86cac6836502aaeb5c894bff6427e837c52c09e0/packages/react-native/Libraries/Core/Devtools/__tests__/parseHermesStack-test.js#L16)

Changelog: [Internal]

Reviewed By: javache, NickGerleman

Differential Revision: D63659013
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D63659013

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D63659013

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D63659013

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D63659013

RSNara added a commit to RSNara/react-native that referenced this pull request Oct 9, 2024
@RSNara @facebook-github-bot
Align earlyjs c++ stack trace parsing with js (facebook#46894)
34026a5 
Summary:

This diff re-implements [js error stack trace parsing](https://github.com/facebook/react-native/blob/86cac6836502aaeb5c894bff6427e837c52c09e0/packages/react-native/Libraries/Core/Devtools/parseErrorStack.js#L41-L57) in c++.

Details:
- I migrated [stacktrace-parser](https://github.com/errwischt/stacktrace-parser/blob/ad379de5e5ac056012bbeb12923cf502aefe4710/src/stack-trace-parser.js#L7)
- I migrated [parseHermesStack.js](https://github.com/facebook/react-native/blob/86cac6836502aaeb5c894bff6427e837c52c09e0/packages/react-native/Libraries/Core/Devtools/parseHermesStack.js#L82)

I also migrated all their tests to c++:
- [stacktrace-parser tests](https://github.com/errwischt/stacktrace-parser/blob/ad379de5e5ac056012bbeb12923cf502aefe4710/test/stack-trace-parser.spec.js#L5)
- [parseHermesStack tests](https://github.com/facebook/react-native/blob/86cac6836502aaeb5c894bff6427e837c52c09e0/packages/react-native/Libraries/Core/Devtools/__tests__/parseHermesStack-test.js#L16)

Changelog: [Internal]

Reviewed By: javache, NickGerleman

Differential Revision: D63659013
RSNara added a commit to RSNara/react-native that referenced this pull request Oct 9, 2024
@RSNara @facebook-github-bot
Align earlyjs c++ stack trace parsing with js (facebook#46894)
15beaa6 
Summary:

This diff re-implements [js error stack trace parsing](https://github.com/facebook/react-native/blob/86cac6836502aaeb5c894bff6427e837c52c09e0/packages/react-native/Libraries/Core/Devtools/parseErrorStack.js#L41-L57) in c++.

Details:
- I migrated [stacktrace-parser](https://github.com/errwischt/stacktrace-parser/blob/ad379de5e5ac056012bbeb12923cf502aefe4710/src/stack-trace-parser.js#L7)
- I migrated [parseHermesStack.js](https://github.com/facebook/react-native/blob/86cac6836502aaeb5c894bff6427e837c52c09e0/packages/react-native/Libraries/Core/Devtools/parseHermesStack.js#L82)

I also migrated all their tests to c++:
- [stacktrace-parser tests](https://github.com/errwischt/stacktrace-parser/blob/ad379de5e5ac056012bbeb12923cf502aefe4710/test/stack-trace-parser.spec.js#L5)
- [parseHermesStack tests](https://github.com/facebook/react-native/blob/86cac6836502aaeb5c894bff6427e837c52c09e0/packages/react-native/Libraries/Core/Devtools/__tests__/parseHermesStack-test.js#L16)

Changelog: [Internal]

Reviewed By: javache, NickGerleman

Differential Revision: D63659013
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D63659013

RSNara added a commit to RSNara/react-native that referenced this pull request Oct 9, 2024
@RSNara @facebook-github-bot
Align earlyjs c++ stack trace parsing with js (facebook#46894)
2f9881f 
Summary:

This diff re-implements [js error stack trace parsing](https://github.com/facebook/react-native/blob/86cac6836502aaeb5c894bff6427e837c52c09e0/packages/react-native/Libraries/Core/Devtools/parseErrorStack.js#L41-L57) in c++.

Details:
- I migrated [stacktrace-parser](https://github.com/errwischt/stacktrace-parser/blob/ad379de5e5ac056012bbeb12923cf502aefe4710/src/stack-trace-parser.js#L7)
- I migrated [parseHermesStack.js](https://github.com/facebook/react-native/blob/86cac6836502aaeb5c894bff6427e837c52c09e0/packages/react-native/Libraries/Core/Devtools/parseHermesStack.js#L82)

I also migrated all their tests to c++:
- [stacktrace-parser tests](https://github.com/errwischt/stacktrace-parser/blob/ad379de5e5ac056012bbeb12923cf502aefe4710/test/stack-trace-parser.spec.js#L5)
- [parseHermesStack tests](https://github.com/facebook/react-native/blob/86cac6836502aaeb5c894bff6427e837c52c09e0/packages/react-native/Libraries/Core/Devtools/__tests__/parseHermesStack-test.js#L16)

Changelog: [Internal]

Reviewed By: javache, NickGerleman

Differential Revision: D63659013
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D63659013

RSNara added a commit to RSNara/react-native that referenced this pull request Oct 10, 2024
@RSNara @facebook-github-bot
Align earlyjs c++ stack trace parsing with js (facebook#46894)
f34a5b4 
Summary:

This diff re-implements [js error stack trace parsing](https://github.com/facebook/react-native/blob/86cac6836502aaeb5c894bff6427e837c52c09e0/packages/react-native/Libraries/Core/Devtools/parseErrorStack.js#L41-L57) in c++.

Details:
- I migrated [stacktrace-parser](https://github.com/errwischt/stacktrace-parser/blob/ad379de5e5ac056012bbeb12923cf502aefe4710/src/stack-trace-parser.js#L7)
- I migrated [parseHermesStack.js](https://github.com/facebook/react-native/blob/86cac6836502aaeb5c894bff6427e837c52c09e0/packages/react-native/Libraries/Core/Devtools/parseHermesStack.js#L82)

I also migrated all their tests to c++:
- [stacktrace-parser tests](https://github.com/errwischt/stacktrace-parser/blob/ad379de5e5ac056012bbeb12923cf502aefe4710/test/stack-trace-parser.spec.js#L5)
- [parseHermesStack tests](https://github.com/facebook/react-native/blob/86cac6836502aaeb5c894bff6427e837c52c09e0/packages/react-native/Libraries/Core/Devtools/__tests__/parseHermesStack-test.js#L16)

Changelog: [Internal]

Reviewed By: javache, NickGerleman

Differential Revision: D63659013
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D63659013

@RSNara @facebook-github-bot
easy: Align earlyjs c++ native data structures with js
d682b31 
Summary:
For the js error handling pipeline, the javascript data structure looks like [this](https://www.internalfb.com/code/fbsource/[6181b57f4ba3619f58056bcec65382650d6ff59a]/xplat/js/react-native-github/packages/react-native/src/private/specs/modules/NativeExceptionsManager.js?lines=17-35):

```
export type StackFrame = {|
  column: ?number,
  file: ?string,
  lineNumber: ?number,
  methodName: string,
  collapse?: boolean,
|};
export type ExceptionData = {
  message: string,
  originalMessage: ?string,
  name: ?string,
  componentStack: ?string,
  stack: Array<StackFrame>,
  id: number,
  isFatal: boolean,
  // flowlint-next-line unclear-type:off
  extraData?: Object,
  ...
};
```

So, I made the c++ data structure look similar
```
  struct ParsedError {
    struct StackFrame {
      std::optional<std::string> file;
      std::string methodName;
      std::optional<int> lineNumber;
      std::optional<int> column;
    };

    std::string message;
    std::optional<std::string> originalMessage;
    std::optional<std::string> name;
    std::optional<std::string> componentStack;
    std::vector<StackFrame> stack;
    int id;
    bool isFatal;
    jsi::Object extraData;
  };
```

Notes:
* [parseErrorStack](https://fburl.com/code/e27q9gkc) doesn't actually generate a collapse property on the error object. So, I omitted it from the c++.
* ExceptionsManager [always provides an extraData field](https://fburl.com/code/2bvcsxac). So, I made it required.
* In C++, I just stored extraData as a jsi::Object. I wanted the freedom to store arbitrary key/value pairs. But, I also didn't want to use folly::dynamic.

Changelog: [Internal]

Reviewed By: alanleedev

Differential Revision: D63929580
@RSNara @facebook-github-bot
Align earlyjs c++ stack trace parsing with js (facebook#46894)
7ee42cd 
Summary:

This diff re-implements [js error stack trace parsing](https://github.com/facebook/react-native/blob/86cac6836502aaeb5c894bff6427e837c52c09e0/packages/react-native/Libraries/Core/Devtools/parseErrorStack.js#L41-L57) in c++.

Details:
- I migrated [stacktrace-parser](https://github.com/errwischt/stacktrace-parser/blob/ad379de5e5ac056012bbeb12923cf502aefe4710/src/stack-trace-parser.js#L7)
- I migrated [parseHermesStack.js](https://github.com/facebook/react-native/blob/86cac6836502aaeb5c894bff6427e837c52c09e0/packages/react-native/Libraries/Core/Devtools/parseHermesStack.js#L82)

I also migrated all their tests to c++:
- [stacktrace-parser tests](https://github.com/errwischt/stacktrace-parser/blob/ad379de5e5ac056012bbeb12923cf502aefe4710/test/stack-trace-parser.spec.js#L5)
- [parseHermesStack tests](https://github.com/facebook/react-native/blob/86cac6836502aaeb5c894bff6427e837c52c09e0/packages/react-native/Libraries/Core/Devtools/__tests__/parseHermesStack-test.js#L16)

Changelog: [Internal]

Reviewed By: javache, NickGerleman

Differential Revision: D63659013
RSNara added a commit to RSNara/react-native that referenced this pull request Oct 10, 2024
@RSNara @facebook-github-bot
Align earlyjs c++ stack trace parsing with js (facebook#46894)
8af8833 
Summary:

This diff re-implements [js error stack trace parsing](https://github.com/facebook/react-native/blob/86cac6836502aaeb5c894bff6427e837c52c09e0/packages/react-native/Libraries/Core/Devtools/parseErrorStack.js#L41-L57) in c++.

Details:
- I migrated [stacktrace-parser](https://github.com/errwischt/stacktrace-parser/blob/ad379de5e5ac056012bbeb12923cf502aefe4710/src/stack-trace-parser.js#L7)
- I migrated [parseHermesStack.js](https://github.com/facebook/react-native/blob/86cac6836502aaeb5c894bff6427e837c52c09e0/packages/react-native/Libraries/Core/Devtools/parseHermesStack.js#L82)

I also migrated all their tests to c++:
- [stacktrace-parser tests](https://github.com/errwischt/stacktrace-parser/blob/ad379de5e5ac056012bbeb12923cf502aefe4710/test/stack-trace-parser.spec.js#L5)
- [parseHermesStack tests](https://github.com/facebook/react-native/blob/86cac6836502aaeb5c894bff6427e837c52c09e0/packages/react-native/Libraries/Core/Devtools/__tests__/parseHermesStack-test.js#L16)

Changelog: [Internal]

Reviewed By: javache, NickGerleman

Differential Revision: D63659013
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D63659013

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Oct 10, 2024
@facebook-github-bot
Copy link
Contributor

This pull request has been merged in 934af0c.

This was referenced Nov 12, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported Merged This PR has been merged. p: Facebook Partner: Facebook Partner
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@RSNara @facebook-github-bot