-
Notifications
You must be signed in to change notification settings - Fork 49
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
Fix misaligned lines in debugger #634
Changes from 3 commits
1d72d67
c64d42b
e2cbc1d
900205f
1912a1e
8c309a1
4f37c39
e590d62
d5b61ea
fb61f3c
d28b796
7ecfcb5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,8 @@ | |
Source, | ||
StackFrame, | ||
} from "@vscode/debugadapter"; | ||
import { getReactNativeVersion } from "../utilities/reactNative"; | ||
import semver from "semver"; | ||
import { DebugProtocol } from "@vscode/debugprotocol"; | ||
import WebSocket from "ws"; | ||
import { NullablePosition, SourceMapConsumer } from "source-map"; | ||
|
@@ -83,7 +85,8 @@ | |
private absoluteProjectPath: string; | ||
private projectPathAlias?: string; | ||
private threads: Array<Thread> = []; | ||
private sourceMaps: Array<[string, string, SourceMapConsumer]> = []; | ||
private sourceMaps: Array<[string, string, SourceMapConsumer, number]> = []; | ||
private lineOffset: number; | ||
|
||
private linesStartAt1 = true; | ||
private columnsStartAt1 = true; | ||
|
@@ -96,6 +99,7 @@ | |
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This parameter should really be called |
||
|
||
this.connection.on("open", () => { | ||
// the below catch handler is used to ignore errors coming from non critical CDP messages we | ||
|
@@ -150,7 +154,22 @@ | |
const decodedData = Buffer.from(base64Data, "base64").toString("utf-8"); | ||
const sourceMap = JSON.parse(decodedData); | ||
const consumer = await new SourceMapConsumer(sourceMap); | ||
this.sourceMaps.push([message.params.url, message.params.scriptId, consumer]); | ||
|
||
// This line is here because of a problem with sourcemaps for expo projects, | ||
// that was addressed in this PR https://github.com/expo/expo/pull/29463, | ||
// unfortunately it still requires changes to metro that were attempted here | ||
// 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. | ||
kmagiera marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// more over offset should only be applied to the main bundle sourcemap as other files | ||
// (generated during reload) do not contain the prelude causing the issue | ||
const shouldApplyOffset = | ||
kmagiera marked this conversation as resolved.
Show resolved
Hide resolved
|
||
semver.lte(getReactNativeVersion(), "0.76.0") && this.sourceMaps.length === 0; | ||
this.sourceMaps.push([ | ||
message.params.url, | ||
message.params.scriptId, | ||
consumer, | ||
shouldApplyOffset ? this.lineOffset : 0, | ||
kmagiera marked this conversation as resolved.
Show resolved
Hide resolved
|
||
]); | ||
this.updateBreakpointsInSource(message.params.url, consumer); | ||
} | ||
|
||
|
@@ -264,7 +283,7 @@ | |
let sourceLine1Based = lineNumber1Based; | ||
let sourceColumn0Based = columnNumber0Based; | ||
|
||
this.sourceMaps.forEach(([url, id, consumer]) => { | ||
this.sourceMaps.forEach(([url, id, consumer, lineOffset]) => { | ||
// when we identify script by its URL we need to deal with a situation when the URL is sent with a different | ||
// hostname and port than the one we have registered in the source maps. The reason for that is that the request | ||
// that populates the source map (scriptParsed) is sent by metro, while the requests from breakpoints or logs | ||
|
@@ -273,16 +292,16 @@ | |
if (id === scriptIdOrURL || compareIgnoringHost(url, scriptIdOrURL)) { | ||
scriptURL = url; | ||
const pos = consumer.originalPositionFor({ | ||
line: lineNumber1Based, | ||
line: lineNumber1Based - lineOffset, | ||
column: columnNumber0Based, | ||
}); | ||
if (pos.source != null) { | ||
sourceURL = pos.source; | ||
} | ||
if (pos.line != null) { | ||
sourceLine1Based = pos.line; | ||
} | ||
if (pos.column != null) { | ||
sourceColumn0Based = pos.column; | ||
} | ||
} | ||
|
@@ -440,7 +459,7 @@ | |
} | ||
let position: NullablePosition = { line: null, column: null, lastColumn: null }; | ||
let originalSourceURL: string = ""; | ||
this.sourceMaps.forEach(([sourceURL, scriptId, consumer]) => { | ||
this.sourceMaps.forEach(([sourceURL, scriptId, consumer, lineOffset]) => { | ||
const sources = []; | ||
consumer.eachMapping((mapping) => { | ||
sources.push(mapping.source); | ||
|
@@ -451,9 +470,9 @@ | |
column: columnNumber0Based, | ||
bias: SourceMapConsumer.LEAST_UPPER_BOUND, | ||
}); | ||
if (pos.line != null) { | ||
originalSourceURL = sourceURL; | ||
position = pos; | ||
position = { ...pos, line: pos.line + lineOffset }; | ||
} | ||
}); | ||
if (position.line === null) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,7 +8,7 @@ | |
import { shouldUseExpoCLI } from "../utilities/expoCli"; | ||
import { Devtools } from "./devtools"; | ||
import { getLaunchConfiguration } from "../utilities/launchConfiguration"; | ||
import WebSocket from "ws"; | ||
|
||
export interface MetroDelegate { | ||
onBundleError(): void; | ||
|
@@ -46,6 +46,10 @@ | |
transformedFileCount: number; | ||
totalFileCount: number; | ||
} | ||
| { | ||
type: "RNIDE_Expo_Env_Prelude_Lines"; | ||
expoEnvPreludeLines: number; | ||
} | ||
| { | ||
type: "RNIDE_initialize_done"; | ||
port: number; | ||
|
@@ -64,6 +68,7 @@ | |
export class Metro implements Disposable { | ||
private subprocess?: ChildProcess; | ||
private _port = 0; | ||
private _initialBundleLineOffset = 0; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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" |
||
private startPromise: Promise<void> | undefined; | ||
private usesNewDebugger?: Boolean; | ||
|
||
|
@@ -80,6 +85,10 @@ | |
return this._port; | ||
} | ||
|
||
public get initialBundleLineOffset() { | ||
return this._initialBundleLineOffset; | ||
} | ||
|
||
public dispose() { | ||
this.subprocess?.kill(9); | ||
} | ||
|
@@ -207,6 +216,12 @@ | |
} | ||
|
||
switch (event.type) { | ||
case "RNIDE_Expo_Env_Prelude_Lines": | ||
this._initialBundleLineOffset = event.expoEnvPreludeLines; | ||
Logger.debug( | ||
`Metro initial bundle offset is set to ${this._initialBundleLineOffset}` | ||
); | ||
break; | ||
case "RNIDE_initialize_done": | ||
this._port = event.port; | ||
Logger.info(`Metro started on port ${this._port}`); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency with RNIDE_initialize_done I wouldn't use capitalized words here