Fix misaligned lines in debugger #634
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
I don't like the coupling between console calls that are also meant to provide the offset and the source map parser that seems to expect the console call prior to the "source maps ready" call.
I think we should instead restructure it in the following way:
- Remove
__EXPO_ENV_PRELUDE_LINES__entirely - Instead of it, we should log a command from metro process that will be captured by line reader. Similarily to how we do with RNIDE_initialize_done, but it will also contain the offset number
- Read the offset in metro.ts using line reader and expose it such that we can provide it as a parameter to the debug session
|
@kmagiera you were right, I changed it according to your suggestions. |
| module.output[0].data.code = `${expoEnvCode};var __EXPO_ENV_PRELUDE_LINES__=${module.output[0].data.lineCount};`; | ||
| } | ||
| const expoEnvPreludeLines = module.output[0].data.lineCount; | ||
| process.stdout.write(JSON.stringify({ type: "RNIDE_Expo_Env_Prelude_Lines", expoEnvPreludeLines })); |
There was a problem hiding this comment.
For consistency with RNIDE_initialize_done I wouldn't use capitalized words here
| @@ -64,6 +68,7 @@ type MetroEvent = | |||
| export class Metro implements Disposable { | |||
| private subprocess?: ChildProcess; | |||
| private _port = 0; | |||
| private _initialBundleLineOffset = 0; | |||
There was a problem hiding this comment.
I don't like "initial" word here as it isn't accurate. I think the offset applies for the whole bundle not just for the first bundle that gets loaded as it is currently implemented.
We should rename "mainBundleLineOffset"
|
I'm now thinking that it may actually be the easiest solution to still include the offset in the bundle, and when we get source map we'd check if the offset variable exists there and read its value. If this would work, that'd be the best solution in my option. The only reason why it may not work is that I'm not sure if the preamble actually gets included in the source map at all. Tthe reason we're adding this offset was that there was an issue with that. But, perhaps, the issue was only because it is considered to have 0 length rather than just being completely omitted. You'll need to check the bundle for that to tell |
There was a problem hiding this comment.
Left a few more comments inline, but since only related to variable/parameter names and comments I'm accepting, as otherwise the code looks good.
The only comment that may require more attention is about whether we should test agains expo or react native version when determining when to use offset parameter and when not
|
|
||
| // This is a heuristic that checks if the source map should contain __env__ | ||
| // module that is added by expo, but not reported in the source map | ||
| const isFileWithOffset = sourceMap.sources.includes("__prelude__"); |
There was a problem hiding this comment.
rename isMainBundle or isFullBundle
| @@ -96,6 +98,7 @@ export class DebugAdapter extends DebugSession { | |||
| this.absoluteProjectPath = configuration.absoluteProjectPath; | |||
| this.projectPathAlias = configuration.projectPathAlias; | |||
| this.connection = new WebSocket(configuration.websocketAddress); | |||
| this.lineOffset = configuration.lineOffset; | |||
There was a problem hiding this comment.
This parameter should really be called expoPreludeExtraLines or something along these lines instead of being so generic
| // https://github.com/facebook/metro/pull/1284 we should monitor the situation | ||
| // in upcoming versions and if the changes are still not added bump the version below. | ||
| const shouldApplyOffset = | ||
| semver.lte(getReactNativeVersion(), "0.76.0") && isFileWithOffset; |
There was a problem hiding this comment.
should we check for expo version here and not react native?
kmagiera
deleted the
@Filip131311/FixDebuggerMissalinedLinesOnExpoProjects
branch
This PR fixes a problem with debugger on expo projects, caused by prelude lines added to the entry bundle file added by expo. The solution is t0 add the required offset to the prelude causing the problem and scanning for it when receiving source map in the debuger.
It is possible that the changes will no longer be needed in future versions of react native as the expo team tries to correct source map generation to include extra lines, for more information read those PRs:
This is why we add version check before adding lineOffset to initial source map.
How Has This Been Tested:
Additional changes:
This PR also fixes a minor mistake with
getReactNativeVersionutility that does not need to be async and as it is used, we make it synchronous as part of this PR.