Add more internal callsites for better stack frames #1699

rickhanlonii · 2022-09-08T15:18:23Z

Summary:

Adds more internal callsites to clean up log box internal stack frame collapsing.

Test Plan:

Before

Notice how the code frame is not the error in the user code:

After

Now it is:

thymikee · 2022-09-08T15:24:21Z

cc @motiz88 @huntie, mind reviewing that?
@rickhanlonii can you run yarn lint --fix?

rickhanlonii · 2022-09-08T17:20:30Z

@motiz88 @huntie I'm also curious - do we know why the stack frame says "" and "global code"? Is there something more relevant we can display there or is it from Hermes? And should we collapse the .bundle frames by default (the second frame in the "after" image)?

motiz88 · 2022-09-08T21:41:55Z

I'm not sure where the string global code is coming from (engine-specific?) but this actually seems pretty informative: it points to the error being in the initialisation path, and the .bundle means this is generated code with no source mapping. We could somewhat reasonably map this particular case to somewhere in the bundle's entry point, but technically that frame does live outside of that module, so it's an ambiguous symbolication and I'd avoid it.

I would also not collapse all .bundle frames ( = all unsymbolicated frames) as it's a bit too indiscriminate, especially for cases where symbolication might be broken for whatever reason. But maybe matching the global code string (and its equivalents in other engines) alongside .bundle is a good heuristic. Unsymbolicated global code = most likely bundle infrastructure that can't be mapped to any file on disk.

motiz88 · 2022-09-08T21:43:36Z

packages/cli-plugin-metro/src/tools/loadMetroConfig.ts

@@ -14,9 +14,14 @@ const INTERNAL_CALLSITES_REGEX = new RegExp(
    '/Libraries/YellowBox/.+\\.js$',
    '/Libraries/LogBox/.+\\.js$',
    '/Libraries/Core/Timers/.+\\.js$',
+    '/Libraries/WebSocket/.+\\.js',


Suggested change

'/Libraries/WebSocket/.+\\.js',

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

motiz88 · 2022-09-08T21:43:54Z

packages/cli-plugin-metro/src/tools/loadMetroConfig.ts

@@ -14,9 +14,14 @@ const INTERNAL_CALLSITES_REGEX = new RegExp(
    '/Libraries/YellowBox/.+\\.js$',
    '/Libraries/LogBox/.+\\.js$',
    '/Libraries/Core/Timers/.+\\.js$',
+    '/Libraries/WebSocket/.+\\.js',
+    '/Libraries/vendor/.+\\.js',


Suggested change

'/Libraries/vendor/.+\\.js',

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

motiz88 · 2022-09-08T21:46:11Z

packages/cli-plugin-metro/src/tools/loadMetroConfig.ts

    '/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\\]',


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.

motiz88 · 2022-09-08T21:47:12Z

packages/cli-plugin-metro/src/tools/loadMetroConfig.ts

@@ -14,9 +14,14 @@ const INTERNAL_CALLSITES_REGEX = new RegExp(
    '/Libraries/YellowBox/.+\\.js$',


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

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

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

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.

A wild Nakazawa appears!

Christoph used Context.

It's super effective!

motiz88 · 2022-09-08T21:50:41Z

I might be missing something but how does the invariant stack frame get skipped? Not seeing any pattern that specifically targets it.

rickhanlonii · 2022-09-09T02:01:27Z

Nice catch, I missed this copying it over.

thymikee

Thanks @rickhanlonii and @motiz88

Summary: In 2017, React published v15.5 which extracted the built-in `prop types` to a separate package to reflect the fact that not everybody uses them. In 2018, React Native started to remove `PropTypes` from React Native for the same reason. In 0.68 React Native introduced a deprecation warning which notified users that the change was coming, and in 0.69 we removed the PropTypes entirely. The feedback we've received from the community is that there has not been enough time to migrate libraries off of PropTypes. This has resulted in users needing to patch the React Native package `index.js` file directly to add back the PropTypes, instead of migrating off of them. We can empathize with this fix short term (it unblocks the upgrade) but long term this patch will cause users to miss important changes to `index.js`, and add a maintenance cost for users. Part of the reason there was not enough time is that we didn't do a good job surfacing libraries that were using PropTypes. This means, when you got a deprecation warning, it wasn't clear where the source of the usage was (either in your code or in a library). So even if you wanted to migrate, it was difficult to know where to actually make the change. In the next release, we've made it easier to find call sites using deprecated types by [fixing the code frame in errors](react-native-community/cli#1699) reporting in LogBox, and ensuring that [the app doesn't crash without a warning](#34650). This should make it easier to identify exactly where the deprecated usage is, so you can migrate it. To help users get off of the patch, and allow more time to migrate, we're walking back the removal of PropTypes, and keeping it as a deprecation for a couple more versions. We ask that you either migrate off PropTypes to a type system like TypeScript, or migrate to the `deprecated-react-native-prop-types` package. Once we feel more confident that the community has migrated and will not need to patch React Native in order to fix this issue, we'll remove the PropTypes again. **If you have any trouble finding the source of the PropType usage, please file an issue so we can help track it down with you.** Changelog: [General][Changed] - Add back deprecated PropTypes Reviewed By: yungsters Differential Revision: D40725705 fbshipit-source-id: 8ce61be30343827efd6dc89a012eeef0b6676deb

Summary: In 2017, React published v15.5 which extracted the built-in `prop types` to a separate package to reflect the fact that not everybody uses them. In 2018, React Native started to remove `PropTypes` from React Native for the same reason. In 0.68 React Native introduced a deprecation warning which notified users that the change was coming, and in 0.69 we removed the PropTypes entirely. The feedback we've received from the community is that there has not been enough time to migrate libraries off of PropTypes. This has resulted in users needing to patch the React Native package `index.js` file directly to add back the PropTypes, instead of migrating off of them. We can empathize with this fix short term (it unblocks the upgrade) but long term this patch will cause users to miss important changes to `index.js`, and add a maintenance cost for users. Part of the reason there was not enough time is that we didn't do a good job surfacing libraries that were using PropTypes. This means, when you got a deprecation warning, it wasn't clear where the source of the usage was (either in your code or in a library). So even if you wanted to migrate, it was difficult to know where to actually make the change. In the next release, we've made it easier to find call sites using deprecated types by [fixing the code frame in errors](react-native-community/cli#1699) reporting in LogBox, and ensuring that [the app doesn't crash without a warning](facebook#34650). This should make it easier to identify exactly where the deprecated usage is, so you can migrate it. To help users get off of the patch, and allow more time to migrate, we're walking back the removal of PropTypes, and keeping it as a deprecation for a couple more versions. We ask that you either migrate off PropTypes to a type system like TypeScript, or migrate to the `deprecated-react-native-prop-types` package. Once we feel more confident that the community has migrated and will not need to patch React Native in order to fix this issue, we'll remove the PropTypes again. **If you have any trouble finding the source of the PropType usage, please file an issue so we can help track it down with you.** Changelog: [General][Changed] - Add back deprecated PropTypes Reviewed By: yungsters Differential Revision: D40725705 fbshipit-source-id: 8ce61be30343827efd6dc89a012eeef0b6676deb

Add more internal callsites

597cb24

rickhanlonii requested review from grabbou, thymikee and Esemesek as code owners September 8, 2022 15:18

fix lint

b6fb4b2

motiz88 reviewed Sep 8, 2022

View reviewed changes

Add invariant and react-native/index.js

e6ca235

thymikee approved these changes Sep 9, 2022

View reviewed changes

thymikee changed the title ~~Add more internal callsites~~ Add more internal callsites for better stack frames Sep 9, 2022

thymikee merged commit 58ca7f1 into react-native-community:main Sep 9, 2022

rickhanlonii deleted the rh-metro-config branch September 13, 2022 01:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more internal callsites for better stack frames #1699

Add more internal callsites for better stack frames #1699

rickhanlonii commented Sep 8, 2022

thymikee commented Sep 8, 2022

rickhanlonii commented Sep 8, 2022

motiz88 commented Sep 8, 2022

motiz88 Sep 8, 2022

motiz88 Sep 8, 2022

motiz88 Sep 8, 2022

motiz88 Sep 8, 2022

rickhanlonii Sep 9, 2022

thymikee Sep 9, 2022

cpojer Sep 9, 2022

rickhanlonii Sep 9, 2022

motiz88 commented Sep 8, 2022

rickhanlonii commented Sep 9, 2022

thymikee left a comment

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

		@@ -14,9 +14,14 @@ const INTERNAL_CALLSITES_REGEX = new RegExp(
		'/Libraries/YellowBox/.+\\.js$',

Add more internal callsites for better stack frames #1699

Add more internal callsites for better stack frames #1699

Conversation

rickhanlonii commented Sep 8, 2022

Summary:

Test Plan:

Before

After

thymikee commented Sep 8, 2022

rickhanlonii commented Sep 8, 2022

motiz88 commented Sep 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

motiz88 commented Sep 8, 2022

rickhanlonii commented Sep 9, 2022

thymikee left a comment

Choose a reason for hiding this comment