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

Add more internal callsites for better stack frames #1699

Merged
merged 3 commits into from
Sep 9, 2022
Merged

Add more internal callsites for better stack frames #1699

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
597cb24
Add more internal callsites
rickhanlonii Sep 8, 2022
b6fb4b2
fix lint
rickhanlonii Sep 8, 2022
e6ca235
Add invariant and react-native/index.js
rickhanlonii Sep 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/cli-plugin-metro/src/tools/loadMetroConfig.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,14 @@ const INTERNAL_CALLSITES_REGEX = new RegExp(
'/Libraries/YellowBox/.+\\.js$',
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if all of these should be prefixed with react-native. (Not sure)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah probably -- I opted to minimize the diff because I wasn't sure why it was this way.

Copy link
Member

Choose a reason for hiding this comment

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

@rickhanlonii mind following up with a PR with adjusted paths?

Copy link
Member

Choose a reason for hiding this comment

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

The reason the react-native prefix is missing is because forks of React Native such as react-native-tvos (and react-native-windows?) have copies of these files but are installed within node_modules using their forked name. If you match on react-native, you'll break YellowBox for all forks. You might want to use node_modules\/(react-native(?:-(.*?))?)\/ here and below where you added the match on /node_modules/react-native/index.js.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A wild Nakazawa appears!

Christoph used Context.

It's super effective!

'/Libraries/LogBox/.+\\.js$',
'/Libraries/Core/Timers/.+\\.js$',
'/Libraries/WebSocket/.+\\.js',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
'/Libraries/WebSocket/.+\\.js',
'/Libraries/WebSocket/.+\\.js$',

'/Libraries/vendor/.+\\.js',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
'/Libraries/vendor/.+\\.js',
'/Libraries/vendor/.+\\.js$',

'/node_modules/react-devtools-core/.+\\.js$',
'/node_modules/react-refresh/.+\\.js$',
'/node_modules/scheduler/.+\\.js$',
'/node_modules/event-target-shim/.+\\.js$',
'/metro-runtime/.+\\.js$',
'\\[native code\\]',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
'\\[native code\\]',
'^\\[native code\\]$',

I suspect this string may be engine-specific - have you tested this on both JSC and Hermes?

Also, bikeshedding: might be useful to include native code as intermediate frames, though I agree they're less useful at the top since we have no JS code frame to show there.

].join('|'),
);

Expand Down