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.

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

String that looks like a source map comment gets used as a source map comment, but only in ESM #51522

Closed
nicolo-ribaudo opened this issue Jan 19, 2024 · 4 comments
Labels
source maps Issues and PRs related to source map support.

Comments

@nicolo-ribaudo
Copy link
Contributor

nicolo-ribaudo commented Jan 19, 2024

Version

21.6.0

Platform

Darwin Nics-MacBook-Air.local 23.2.0 Darwin Kernel Version 23.2.0: Wed Nov 15 21:59:33 PST 2023; root:xnu-10002.61.3~2/RELEASE_ARM64_T8112 arm64

Subsystem

No response

What steps will reproduce the bug?

Input code:

"use strict";

throw new Error("Hello world!");`
//# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJmaWxlIjoiIiwic291cmNlUm9vdCI6IiIsInNvdXJjZXMiOlsiaHR0cDovL2xvY2FsaG9zdDo0NTQ1L3J1bi9pbmxpbmVfanNfc291cmNlX21hcF8yLnRzIl0sInNvdXJjZXNDb250ZW50IjpbIjErMTtcbmludGVyZmFjZSBUZXN0IHtcbiAgaGVsbG86IHN0cmluZztcbn1cblxudGhyb3cgbmV3IEVycm9yKFwiSGVsbG8gd29ybGQhXCIgYXMgdW5rbm93biBhcyBzdHJpbmcpO1xuIl0sIm5hbWVzIjpbXSwibWFwcGluZ3MiOiI7QUFBQSxDQUFDLEdBQUMsQ0FBQyxDQUFDO0FBS0osTUFBTSxJQUFJLEtBQUssQ0FBQyxjQUErQixDQUFDLENBQUMifQ==
console.log(1);
// `

Output:

 node --enable-source-maps main2.mjs                                                                 
file:///Users/nic/Documents/dev/github.com/tc39/source-map-spec/example/main2.mjs:3
throw new Error("Hello world!");`
      ^

Error: Hello world!
    at <anonymous> (http://localhost:4545/run/inline_js_source_map_2.ts:6:7)
    at ModuleJob.run (node:internal/modules/esm/module_job:218:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:323:24)
    at async loadESM (node:internal/process/esm_loader:28:7)
    at async handleMainPromise (node:internal/modules/run_main:120:12)

Node.js v21.6.0

As you can see, the stack trace points to a non-existent .ts file

How often does it reproduce? Is there a required condition?

If you ran that code as CJS rather than ESM, no source map is applied (as expected):

 node --enable-source-maps main2.js 
/Users/nic/Documents/dev/github.com/tc39/source-map-spec/example/main2.js:3
throw new Error("Hello world!");`
^

Error: Hello world!
    at Object.<anonymous> (/Users/nic/Documents/dev/github.com/tc39/source-map-spec/example/main2.js:3:7)
    at Module._compile (node:internal/modules/cjs/loader:1378:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1437:10)
    at Module.load (node:internal/modules/cjs/loader:1212:32)
    at Module._load (node:internal/modules/cjs/loader:1028:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:142:12)
    at node:internal/main/run_main_module:28:49

Node.js v21.6.0

Note that source maps in CJS in general work: you can test by deleting the backtick after new Error("Hello world!"); (effectively turning the string-that-looks-like-a-comment into an actual source map comments) and you'll see that the source map is applied

Additional information

Some discussion is happening here: tc39/source-map#64

@nicolo-ribaudo
Copy link
Contributor Author

I tried reading through the code, and I think the different behavior is because Node.js uses V8's GetSourceMappingURL() API for scripts, but a simple regexp for modules.

@aduh95
Copy link
Contributor

aduh95 commented Jan 19, 2024

Can this be fixed without adding a full fledge ES parser? IMO the increase in correctness would not be worth the decrease of performance such parser would impose – also I'd expect that no one is affected by this bug, but you never know 😅

@targos
Copy link
Member

targos commented Jan 19, 2024

V8's GetSourceMappingURL() API is also available on modules.

@legendecas legendecas added the source maps Issues and PRs related to source map support. label Feb 7, 2024
legendecas pushed a commit that referenced this issue Feb 26, 2024
PR-URL: #51690
Refs: #51522
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@legendecas
Copy link
Member

Fixed in #51690

marco-ippolito pushed a commit that referenced this issue Feb 26, 2024
PR-URL: #51690
Refs: #51522
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
marco-ippolito pushed a commit that referenced this issue Feb 26, 2024
PR-URL: #51690
Refs: #51522
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
marco-ippolito pushed a commit that referenced this issue Feb 27, 2024
PR-URL: #51690
Refs: #51522
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
richardlau pushed a commit that referenced this issue Mar 25, 2024
PR-URL: #51690
Refs: #51522
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
richardlau pushed a commit that referenced this issue Mar 25, 2024
PR-URL: #51690
Refs: #51522
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
rdw-msft pushed a commit to rdw-msft/node that referenced this issue Mar 26, 2024
PR-URL: nodejs#51690
Refs: nodejs#51522
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
source maps Issues and PRs related to source map support.
Projects
None yet
Development

No branches or pull requests

4 participants