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

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

Closed
wants to merge 1 commit into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Feb 12, 2022

To align with CJS behavior.

Refs: #41924 (comment)

@aduh95 aduh95 added the esm Issues and PRs related to the ECMAScript Modules implementation. label Feb 12, 2022
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. labels Feb 12, 2022
@bmeck
Copy link
Member

bmeck commented Feb 12, 2022 via email

@bmeck
Copy link
Member

bmeck commented Feb 12, 2022 via email

@aduh95
Copy link
Contributor Author

aduh95 commented Feb 12, 2022

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
Copy link
Contributor

@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
Copy link
Contributor Author

aduh95 commented Feb 12, 2022

@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
Copy link
Contributor

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

@bmeck
Copy link
Member

bmeck commented Feb 13, 2022

@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
Copy link
Contributor Author

aduh95 commented Feb 13, 2022

@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
Copy link
Member

bmeck commented Feb 13, 2022

@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:

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

for ESM and

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
Copy link
Contributor Author

aduh95 commented Feb 19, 2022

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
Copy link
Contributor

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

@bmeck
Copy link
Member

bmeck commented Feb 19, 2022

@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
Copy link
Contributor Author

aduh95 commented Apr 3, 2022

Closing in favor of #42392.

@aduh95 aduh95 closed this Apr 3, 2022
@aduh95 aduh95 deleted the esm-stdin branch April 3, 2022 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants