retain query string in resolved paths

[andrew0]

I'm trying to use tsimp in conjunction with esmock and it's currently not working. esmock registers module hooks that adds some metadata to mocked import paths by adding a query string to the path, but tsimp removes this query in its resolve hook. Adding the query string to the resolved .ts path fixes the integration with esmock.

[andrew0]

Not sure how interested you are in supporting this particular integration, but after applying this fix, another issue I noticed is that the debugger sometimes gets confused about the loaded code's source path. This repo has a demo that triggers the issue: https://github.com/andrew0/tsimp-esmock (run the command node --import=tsimp/import src/index.ts)

If I set a breakpoint on the log function:

VS Code opens up a code blob with the rewritten filename:

It still knows to stop at the correct line, but I expect it to show the breakpoint in the source file. It seems like this is only triggered by setting a breakpoint in a module that is a nested mock, i.e. the module is being mocked, and the parent module that's importing it has also been mocked. If I set a breakpoint in nested.ts, it works as expected:

I was able to fix this problem by replacing the .ts extension with the compiled .js extension in the responseURL in the load hook:
andrew0@b0a56e0
Demo: https://github.com/andrew0/tsimp-esmock/tree/response-url

My thinking here was that the .ts extension is never actually imported directly in the code, so maybe there's some heuristic in the debugger that's getting confused by a completely different filename. I dunno if messing with the extension is really the proper thing to do, though, or why it's only necessary in this nested scenario. I also don't see any documentation for responseURL in the Node docs...

edit: found this nodejs/node#43164

In addition this exposes the responseURL return value to the load hook, which is optional. This is landing as treating that as an undocumented loader API for now, since it's generally not an encouraged pattern but is needed by the core loader piping. Whether it should be a public API is a question for the loaders design more generally.

also, changing the responseURL alters the value of import.meta.url to a non-existent path, so this is probably not the right fix...

[isaacs]

Removing the query in the resolve() hook is likely a problem for tap as well. At least, it's surprising behavior.

But I think the thing to do here is not to put the query string back on the filename. That's a path, paths don't have query strings. I think the thing to do here is to return both the resolved target URL as well as the fileName, so that we don't lose the information, and use that where urls are relevant, rather than ever turning the fileName back into a URL (or just use URLs more consistently everywhere, push the conversion into a path to the last point possible). Otherwise, since /path/to/file?query is a valid filename, file:///path/to/file?query and file:///path/to/file%3Fquery become indistinguishable.

[isaacs]

This seems to avoid the problem, without confusing the debugger or using any undocumented APIs:

diff --git a/src/client.ts b/src/client.ts
index 05fce0e..92b05f2 100644
--- a/src/client.ts
+++ b/src/client.ts
@@ -87,15 +87,16 @@ export class DaemonClient extends SockDaemonClient<
   }
 
   /**
-   * Translate a file like ./src/foo.js into ./src/foo.ts
+   * Translate a module identifier like ./src/foo.js into
+   * file:///path/to/src/foo.ts
    * A file that isn't .ts or isn't a file:// url is returned as-is.
    */
   async resolve(url: string, parentURL?: string): Promise<string> {
-    const { fileName } = (await this.request({
+    const { target } = (await this.request({
       action: 'resolve',
       url,
       parentURL,
     })) as ResolveResult
-    return fileName ?? url
+    return target ?? url
   }
 }
diff --git a/src/hooks/hooks.mts b/src/hooks/hooks.mts
index 7441cb1..6f33843 100644
--- a/src/hooks/hooks.mts
+++ b/src/hooks/hooks.mts
@@ -62,20 +62,30 @@ export const resolve: ResolveHook = async (
         String(new URL(url, parentURL))
       : url
   return nextResolve(
-    target.startsWith('file://') && !target.startsWith(nm)
+    target.startsWith('file://') && !startsWithCS(target, nm)
       ? await getClient().resolve(url, parentURL)
       : url,
     context
   )
 }
 
+// case (in-)sensitive String.startsWith
+const cs =
+  process.platform !== 'win32' && process.platform !== 'darwin'
+/* c8 ignore start */
+const startsWithCS = cs
+  ? (haystack: string, needle: string) => haystack.startsWith(needle)
+  : (haystack: string, needle: string) =>
+      haystack.toUpperCase().startsWith(needle.toUpperCase())
+/* c8 ignore stop */
+
 // ts programs have import filenames like ./x.js, but the source
 // lives in ./x.ts. Find the source and compile it.
 const nm = String(pathToFileURL(pathResolve('node_modules'))) + '/'
 const proj = String(pathToFileURL(process.cwd())) + '/'
 let hookedCJS = false
 export const load: LoadHook = async (url, context, nextLoad) => {
-  if (url.startsWith(proj) && !url.startsWith(nm)) {
+  if (startsWithCS(url, proj) && !startsWithCS(url, nm)) {
     const inputFile = fileURLToPath(url)
     const { fileName, diagnostics } = await getClient().compile(
       inputFile,
diff --git a/src/service/service.ts b/src/service/service.ts
index 13bcd24..0e23ff9 100644
--- a/src/service/service.ts
+++ b/src/service/service.ts
@@ -83,8 +83,18 @@ export class DaemonServer extends SockDaemonServer<
         : url
     if (target.startsWith('file://')) {
       const tsFile = findTsFile(target)
+      if (!tsFile) {
+        return {}
+      }
+
+      const url = new URL(target)
+      url.pathname = pathToFileURL(tsFile).pathname
+
       if (tsFile) {
-        return { fileName: String(pathToFileURL(tsFile)) }
+        return {
+          fileName: tsFile,
+          target: String(url),
+        }
       }
     }
     return {}
diff --git a/src/types.ts b/src/types.ts
index a706ed8..6eecfbd 100644
--- a/src/types.ts
+++ b/src/types.ts
@@ -44,6 +44,7 @@ export type ServiceResolveRequest = MessageBase &
 
 export type ResolveResult = {
   fileName?: string
+  target?: string
 }
 export type ServiceResolveResult = MessageBase &
   ResolveResult & {

[andrew0]

cool, thanks for merging the fix! I'm still seeing the weirdness with the debugger getting confused about the source file, but I believe that's actually an issue with esmock's resolver.

[isaacs]

It might also be an issue with the VSCode debugger? Using node --inspect-brk with the Chrome dev tools debugger gets the right filename/line no problem.