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

module,src: add --experimental-entry-url flag #49975

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Sep 30, 2023

--experimental-entry-url is a new boolean flag that causes the entry point string to be parsed as a URL and loaded by the ESM loader: node --experimental-entry-url --enable-source-maps ./entry.js?foo=bar

Refs: #49432 (comment)

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Sep 30, 2023
Copy link
Contributor

@LiviaMedeiros LiviaMedeiros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to reject relative URLs without leading ./?

Comment on lines +630 to +632
path. The URL must either start with `./` (e.g. `./entry.js`) or be absolute
(e.g. `file:///home/user/entry.js`). Bare specifier (e.g. `entry.js`) won't
work.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part sounds a bit vague: does "won't work" mean throwing an error, or being interpreted as relative URL? Can relative URL start with ../? Would relative URL starting from /, ?, #, %2E/, etc. throw as well?

assert.strictEqual(signal, null);
});

it('should reject loading relative URLs without trailing `./`', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
it('should reject loading relative URLs without trailing `./`', async () => {
it('should reject loading relative URLs without leading `./`', async () => {

execPath,
[
'--experimental-entry-url',
'./printA.js?key=value',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't have to be in this PR, but we'll need to test search and hash being passed correctly and not stripped by fileURLToPath() anywhere.

@GeoffreyBooth
Copy link
Member

I think that node --entry-url entry.js should work, just as new URL('entry.js', url.pathToFileURL(process.cwd())) works. The need for ./ for values passed to --import is to disambiguate from bare specifiers, but the main entry point doesn’t go through the ESM resolution algorithm or resolve bare specifiers.

@@ -617,6 +617,20 @@ files with no extension will be treated as WebAssembly if they begin with the
WebAssembly magic number (`\0asm`); otherwise they will be treated as ES module
JavaScript.

### `--experimental-entry-url`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can call the flag --entry-url, with a printed experimental warning and the docs listing it as experimental. This isn’t a very risky feature that we need the experimental part in the flag name.

} catch {
// Ignore exception
} catch (err) {
if (isMain && !shouldBeTreatedAsRelativeOrAbsolutePath(specifier)) { throw err; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add a comment explaining this?

@GeoffreyBooth GeoffreyBooth added the esm Issues and PRs related to the ECMAScript Modules implementation. label Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. esm Issues and PRs related to the ECMAScript Modules implementation. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants