esm: use [stdin] as module URL when reading stdin

[aduh95]

To align with CJS behavior.

Refs: #41924 (comment)

[nodejs-github-bot]

Review requested:

• @nodejs/modules

[bmeck]

We shouldn't do this. Module url duplication in v8 can cause segfaults. Whatever it is it shouldn't be a real potential file.

…

On Sat, Feb 12, 2022, 9:37 AM Node.js GitHub Bot ***@***.***> wrote: Review requested: - @nodejs/modules <https://github.com/orgs/nodejs/teams/modules> — Reply to this email directly, view it on GitHub <#41946 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABZJIYK7SSLIKQUILDTXJDU2Z5FHANCNFSM5OHA3QWQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you are on a team that was mentioned.Message ID: ***@***.***>

[bmeck]

Noticing eval also is under cwd() ugh

…

On Sat, Feb 12, 2022, 9:39 AM Guy Bedford ***@***.***> wrote: ***@***.**** approved this pull request. — Reply to this email directly, view it on GitHub <#41946 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABZJI6LGPYIYIX455IC6ZLU2Z5MTANCNFSM5OHA3QWQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you are on a team that was mentioned.Message ID: ***@***.***>

[aduh95]

Currently it uses {cwd}/[eval1], this PR changes that to {cwd}/[stdin], I don't think one is more likely to be a file than the other. What would you suggest we should use?

[guybedford]

@aduh95 a useful technique for deduplication here is just to keep a global "eval counter" that is incremented on each eval, then make the ID cwd/[eval${idx}].

[aduh95]

@aduh95 a useful technique for deduplication here is just to keep a global "eval counter" that is incremented on each eval, then make the ID cwd/[eval${idx}].

Well that's already in place, this PR doesn't touch that.

[guybedford]

Ah, there's only one stdin of course, ok seems good to me.

[bmeck]

@aduh95 I've suggested in the past that we move them into their own space so that they could actually work with things like policies. Right now you cannot differentiate a file from stdin for them. Not enough people ever got into the discussion for me to feel sane to move forward with it / I didn't know if back compat was an issue. Since this issue came up back compat is probably not a critical issue since I haven't seen error reports about this.

See: #36472 , which I closed recently due to being unsure, but perhaps I could have left open. That suggested using a url scheme like application:stdin which is used in some other places like android (though the pathnames are not standardized).

[aduh95]

@bmeck It looks like this shape of URL for stdin / eval was added so relative imports can be resolved: #28389
I have no strong feelings regarding how this should be named, and using a dedicated protocol seems a good idea indeed, but that would mean we need to find a workaround to resolve relative imports (or remove support for them on evald code).

[bmeck]

@aduh95 we can make the cache key differ from the resolve pretty simply even if it isn't supported right now. It would need to intercept:

node/lib/internal/modules/esm/loader.js

Line 250 in ceaa299

await this.resolve(specifier, parentURL, importAssertionsForResolve);

for ESM and

node/lib/internal/modules/cjs/loader.js

Line 999 in de677e9

return Module._load(id, this, /* isMain */ false);

for CJS. IDK how people feel about it since both of these kind of are breaking changes. Especially if CJS gets an ID that isn't a file path.

[aduh95]

TIL we don't even set import.meta.url for eval code:

$ echo 'console.log(import.meta.url)' | node --input-type=module
undefined

Sp given that new URL('./local/file', import.meta.url) already doesn't work, if we only want to support relative imports, the solution suggested by Bradley seems like a better path indeed.

[guybedford]

Having a new module record field sounds good to me as well.

[bmeck]

@aduh95 the value of import.meta.url can be set to anything so even if the module is cached at process:eval... the value of import.meta.url could be the cwd + '/'.

[aduh95]

Closing in favor of #42392.