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

Closed
wants to merge 2 commits into 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
@aduh95
Copy link
Contributor Author

aduh95 commented Jun 27, 2024

This has stalled.

@RedYetiDev
Copy link
Member

👋 I implemented this with some slight differences, addressing comments, at #54933

LiviaMedeiros pushed a commit that referenced this pull request Sep 27, 2024
Co-Authored-By: Antoine du Hamel <[email protected]>
PR-URL: #54933
Refs: #49975
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: LiviaMedeiros <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request Oct 4, 2024
Co-Authored-By: Antoine du Hamel <[email protected]>
PR-URL: #54933
Refs: #49975
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: LiviaMedeiros <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request Oct 4, 2024
Co-Authored-By: Antoine du Hamel <[email protected]>
PR-URL: #54933
Refs: #49975
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: LiviaMedeiros <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
Co-Authored-By: Antoine du Hamel <[email protected]>
PR-URL: nodejs#54933
Refs: nodejs#49975
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: LiviaMedeiros <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024
Co-Authored-By: Antoine du Hamel <[email protected]>
PR-URL: nodejs#54933
Refs: nodejs#49975
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: LiviaMedeiros <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
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.

5 participants