esm: improve `fetch_module` test coverage and remove hack #41947

aduh95 · 2022-02-12T15:49:11Z

No description provided.

nodejs-github-bot · 2022-02-12T15:49:15Z

Review requested:

@nodejs/modules

aduh95 · 2022-02-12T17:51:49Z

/cc @nodejs/loaders

GeoffreyBooth

FYI I think this conflicts with #41950; this seems much simpler, and I would recommend that this one land first. cc @benjamingr @JakobJingleheimer

benjamingr · 2022-02-13T08:32:00Z

@GeoffreyBooth why would they conflict they change a different function don’t they?

JakobJingleheimer

Thanks for the improved test coverage! LGTM aside from a couple small non-blockers.

JakobJingleheimer · 2022-02-13T09:36:50Z

lib/internal/modules/esm/fetch_module.js

-        let parent = parentURL;
-        const parentName = path.basename(parent.pathname);
-        if (
-          parentName === '[eval]' ||
-          parentName === '[stdin]'
-        ) parent = 'command-line';
        throw new ERR_NETWORK_IMPORT_DISALLOWED(
          href,
-          parent,
+          parentURL,


The reason I added this was because the output without it seemed likely to confuse (the file path is irrelevant and makes it took like it's coming from a file, but it's not). Aside from consistency with CJS, I think this is not an improvement (I don't feel strongly though).

I added this was because the output without it seemed likely to confuse (the file path is irrelevant and makes it took like it's coming from a file, but it's not)

If the URL is confusing, we should fix the URL itself, not try to hide it in this specific error message imo. I disagree that command-line is less confusing – what if the user didn't start the process via the command-line?

I think this is not an improvement

Note that the condition can never be true: parentName is never equals to [eval] nor [stdin] (it could be [eval1] though, but we're not testing that).

If the URL is confusing, we should fix the URL itself, not try to hide it in this specific error message imo.

Sure! That works for me (and should also better support consistency).

what if the user didn't start the process via the command-line?

Is that possible?

Note that the condition can never be true: parentName is never equals to [eval] nor [stdin] (it could be [eval1] though, but we're not testing that).

If it doesn't now, it did at the time—I specifically tested CLI via -e …, which is how I found this in the first place 🙂 Is there a known related change that would explain why it's not now?

Is that possible?

Sure, why not? I don't have a deep enough knowledge of the various OSs Node.js supports to list them all, but I know that CLI is just an option among others (from the GUI, as a service, from a parent process, etc.).

If it doesn't now, it did at the time—I specifically tested CLI via -e …, which is how I found this in the first place 🙂 Is there a known related change that would explain why it's not now?

Maybe you forgot to add --input-type=module?

Is that possible?

Sure, why not? I don't have a deep enough knowledge of the various OSs Node.js supports to list them all, but I know that CLI is just an option among others (from the GUI, as a service, from a parent process, etc.).

No, I mean, is there a route that exists that leads to [eval*]/[stdin] that did not originate from CLI?

If it doesn't now, it did at the time—I specifically tested CLI via -e …, which is how I found this in the first place 🙂 Is there a known related change that would explain why it's not now?

Maybe you forgot to add --input-type=module?

I did not 🙂

JakobJingleheimer · 2022-02-13T09:39:52Z

test/es-module/test-http-imports-cli.mjs

+    // [ERR_NETWORK_IMPORT_DISALLOWED]: import of 'http://example.com/' by
+    //   …/[eval1] is not supported: http can only be used to load local
+    // resources (use https instead).
+    match(stderr, /[ERR_NETWORK_IMPORT_DISALLOWED]/);


Nit: I prefer the err.code & err.message.includes() used in other ESM error tests. I think consistency would be favourable.

We don't have access to err.code not err.message, we're reading the stderr of a subprocess. FYI this test was inspired from

node/test/es-module/test-esm-import-json-named-export.mjs

Lines 7 to 24 in 15f1a45

const child = spawn(execPath, [

path('es-modules', 'import-json-named-export.mjs'),

]);

let stderr = '';

child.stderr.setEncoding('utf8');

child.stderr.on('data', (data) => {

stderr += data;

});

child.on('close', mustCall((code, _signal) => {

notStrictEqual(code, 0);

// SyntaxError: The requested module '../experimental.json'

// does not provide an export named 'ofLife'

match(stderr, /SyntaxError:/);

match(stderr, /'\.\.\/experimental\.json'/);

match(stderr, /'ofLife'/);

}));

gah, yes indeed (and aligns with #41352)

GeoffreyBooth · 2022-02-13T15:58:15Z

@GeoffreyBooth why would they conflict they change a different function don’t they?

I thought they did, but if they don't then feel free to ignore me.

nodejs-github-bot · 2022-02-19T11:25:49Z

CI: https://ci.nodejs.org/job/node-test-pull-request/42674/

nodejs-github-bot · 2022-02-19T14:03:25Z

CI: https://ci.nodejs.org/job/node-test-pull-request/42676/

nodejs-github-bot · 2022-02-19T16:35:47Z

Landed in 3a1a440

esm: improve fetch_module test coverage and remove hack

3cfd11c

aduh95 added experimental Issues and PRs related to experimental features. loaders Issues and PRs related to ES module loaders labels Feb 12, 2022

nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. labels Feb 12, 2022

GeoffreyBooth approved these changes Feb 13, 2022

View reviewed changes

JakobJingleheimer approved these changes Feb 13, 2022

View reviewed changes

aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Feb 19, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 19, 2022

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 19, 2022

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 19, 2022

nodejs-github-bot merged commit 3a1a440 into nodejs:master Feb 19, 2022

aduh95 deleted the fetch_module-coverage branch February 19, 2022 16:38

This was referenced Feb 20, 2022

CI Reliability 2022-02-20 nodejs/reliability#208

Open

CI Reliability 2022-02-21 nodejs/reliability#209

Open

bengl mentioned this pull request Feb 21, 2022

v17.6.0 release proposal #42072

Merged

github-actions bot mentioned this pull request Feb 22, 2022

CI Reliability 2022-02-22 nodejs/reliability#210

Open

22 tasks

danielleadams mentioned this pull request Apr 24, 2022

v16.15.0 release proposal #42847

Merged

	const child = spawn(execPath, [
	path('es-modules', 'import-json-named-export.mjs'),
	]);

	let stderr = '';
	child.stderr.setEncoding('utf8');
	child.stderr.on('data', (data) => {
	stderr += data;
	});
	child.on('close', mustCall((code, _signal) => {
	notStrictEqual(code, 0);

	// SyntaxError: The requested module '../experimental.json'
	// does not provide an export named 'ofLife'
	match(stderr, /SyntaxError:/);
	match(stderr, /'\.\.\/experimental\.json'/);
	match(stderr, /'ofLife'/);
	}));

Uh oh!

esm: improve fetch_module test coverage and remove hack #41947

esm: improve fetch_module test coverage and remove hack #41947

Uh oh!

Conversation

aduh95 commented Feb 12, 2022

Uh oh!

nodejs-github-bot commented Feb 12, 2022

Uh oh!

aduh95 commented Feb 12, 2022

Uh oh!

GeoffreyBooth left a comment

Choose a reason for hiding this comment

Uh oh!

benjamingr commented Feb 13, 2022

Uh oh!

JakobJingleheimer left a comment

Choose a reason for hiding this comment

Uh oh!

JakobJingleheimer Feb 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduh95 Feb 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JakobJingleheimer Feb 13, 2022

Choose a reason for hiding this comment

Uh oh!

aduh95 Feb 13, 2022

Choose a reason for hiding this comment

Uh oh!

JakobJingleheimer Feb 13, 2022

Choose a reason for hiding this comment

Uh oh!

JakobJingleheimer Feb 13, 2022

Choose a reason for hiding this comment

Uh oh!

aduh95 Feb 13, 2022

Choose a reason for hiding this comment

Uh oh!

JakobJingleheimer Feb 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GeoffreyBooth commented Feb 13, 2022

Uh oh!

nodejs-github-bot commented Feb 19, 2022

Uh oh!

nodejs-github-bot commented Feb 19, 2022

Uh oh!

nodejs-github-bot commented Feb 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

esm: improve `fetch_module` test coverage and remove hack #41947

esm: improve `fetch_module` test coverage and remove hack #41947

JakobJingleheimer Feb 13, 2022 •

edited

Loading

aduh95 Feb 13, 2022 •

edited

Loading

JakobJingleheimer Feb 15, 2022 •

edited

Loading