esm: fix imports from non-file module #42881

aduh95 · 2022-04-27T09:08:50Z

Calling fileURLToPath on a possibly non-file URL throws. Converting to a path is not necessary anyway, as it's already common to see a URL in error messages rather than a path.

Fixes: #42860

Fixes: nodejs#42860

nodejs-github-bot · 2022-04-27T09:08:54Z

Review requested:

@nodejs/modules

JakobJingleheimer · 2022-04-27T09:40:11Z

lib/internal/modules/esm/resolve.js

-    const parentURL = fileURLToPath(parsedParentURL?.href);
-


There was a reason for this (I'm pretty sure not doing this will break something), but I can't remember what it was. @bmeck do you remember?

OH! I partially remember! It's when something is wrong with parsedParentURL—that's why it uses ?.. Without this, the error message was too confusing, like " is not allowed" (where there should be something at the front). Adding it provided an important clue to what is wrong.

We're checking that parsedParentURL is truthy the line just before that, I don't think the ?. could ever take effect. Arguably it's fixing a bigger issue than a confusing error message, I'd be inclined to land this anyway. We can always improve things later if the original problem comes back. wdyt?

GeoffreyBooth

I’m assuming you’ve checked that this is okay with ERR_NETWORK_IMPORT_DISALLOWED.

To answer @JakobJingleheimer’s question, I’d guess we were trying to create a more readable error message, telling the user which file had the issue? And assuming that it would be more expected to tell the user their file as a path rather than as a URL.

JakobJingleheimer · 2022-04-27T16:52:23Z

Maaaybe, but I think it was more than metely pretty-fying. At the least, I think it made the path intelligible in some circumstance it otherwise wasn't. I specifically remember talking to Bradley about this line, but slack ate it (it's beyond the retention age for free Slack 😰).

~~Maybe it had to do with normalising the same error message across CJS and ESM and there was an issue filed this fixed? 🕵️‍♂️~~ nvmd, I was thinking of #39787 which was resolved via a documentation update.

I guess it can't be that important or I would have put a code comment.

JakobJingleheimer · 2022-04-27T17:11:35Z

@aduh95 please see my inline comment. I lost quite a bit of time when I was testing a specific scenario for #36328 because of the missing information in the error message that fileURLToPath() reveals. If needed, I can try to re-discover what the specific repro steps were for this (so it can be addressed in a different way).

aduh95 · 2022-04-27T21:29:42Z

I can try to re-discover what the specific repro steps were for this (so it can be addressed in a different way).

Having a test for that would be ideal 👍 It can also be taken to its own PR as I'd like this patch to land ASAP.

JakobJingleheimer

Sure! That works for me!

nodejs-github-bot · 2022-04-28T17:52:58Z

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

nodejs-github-bot · 2022-04-29T14:04:14Z

Landed in 27ecf1d

Fixes: #42860 PR-URL: #42881 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Guy Bedford <guybedford@gmail.com>

Fixes: nodejs/node#42860 PR-URL: nodejs/node#42881 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Guy Bedford <guybedford@gmail.com>

**What's the problem this PR addresses?** The test added in #5362 doesn't run on Node.js v17. Fixes https://github.com/yarnpkg/berry/actions/runs/5060283241/jobs/9083017466 **How did you fix it?** Updated the range to match Node.js versions containing nodejs/node#42881 **Checklist** - [x] I have read the [Contributing Guide](https://yarnpkg.com/advanced/contributing). - [x] I have set the packages that need to be released for my changes to be effective. - [x] I will check that all automated PR checks pass before the PR gets reviewed.

**What's the problem this PR addresses?** The test added in yarnpkg/berry#5362 doesn't run on Node.js v17. Fixes https://github.com/yarnpkg/berry/actions/runs/5060283241/jobs/9083017466 **How did you fix it?** Updated the range to match Node.js versions containing nodejs/node#42881 **Checklist** - [x] I have read the [Contributing Guide](https://yarnpkg.com/advanced/contributing). - [x] I have set the packages that need to be released for my changes to be effective. - [x] I will check that all automated PR checks pass before the PR gets reviewed.

esm: fix imports from non-file module

38ff8d3

Fixes: nodejs#42860

aduh95 requested a review from JakobJingleheimer April 27, 2022 09:08

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

fixup! esm: fix imports from non-file module

3036627

JakobJingleheimer reviewed Apr 27, 2022

View reviewed changes

GeoffreyBooth approved these changes Apr 27, 2022

View reviewed changes

fixup! esm: fix imports from non-file module

78cf019

JakobJingleheimer approved these changes Apr 27, 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 Apr 27, 2022

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

This comment was marked as outdated.

Sign in to view

github-actions bot mentioned this pull request Apr 29, 2022

CI Reliability 2022-04-29 nodejs/reliability#266

Open

21 tasks

guybedford approved these changes Apr 29, 2022

View reviewed changes

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

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

nodejs-github-bot merged commit 27ecf1d into nodejs:master Apr 29, 2022

aduh95 deleted the fix-import-from-data-modules branch April 29, 2022 14:31

github-actions bot mentioned this pull request Apr 30, 2022

CI Reliability 2022-04-30 nodejs/reliability#267

Open

27 tasks

This was referenced May 1, 2022

CI Reliability 2022-05-01 nodejs/reliability#268

Open

CI Reliability 2022-05-02 nodejs/reliability#269

Open

targos pushed a commit that referenced this pull request May 2, 2022

esm: fix imports from non-file module

a0638a2

Fixes: #42860 PR-URL: #42881 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Guy Bedford <guybedford@gmail.com>

targos mentioned this pull request May 2, 2022

v18.1.0 release proposal #42943

Merged

aduh95 mentioned this pull request May 12, 2022

cannot import built-ins in data: URL module #43060

Closed

juanarbol mentioned this pull request May 31, 2022

2022-05-XX, Version 16.16.0 'Gallium' (LTS) #43268

Closed

targos pushed a commit that referenced this pull request Jul 12, 2022

esm: fix imports from non-file module

0da366a

Fixes: #42860 PR-URL: #42881 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Guy Bedford <guybedford@gmail.com>

targos pushed a commit that referenced this pull request Jul 31, 2022

esm: fix imports from non-file module

6564e69

Fixes: #42860 PR-URL: #42881 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Guy Bedford <guybedford@gmail.com>

targos mentioned this pull request Aug 3, 2022

v16.17.0 release proposal #44098

Merged

CarlesDD mentioned this pull request Sep 2, 2022

Fix module interception on windows nodejs/import-in-the-middle#22

Merged

merceyz mentioned this pull request May 24, 2023

test: fix range for parent URL test yarnpkg/berry#5463

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

esm: fix imports from non-file module #42881

esm: fix imports from non-file module #42881

aduh95 commented Apr 27, 2022

nodejs-github-bot commented Apr 27, 2022

JakobJingleheimer Apr 27, 2022

JakobJingleheimer Apr 27, 2022 •

edited

Loading

aduh95 Apr 27, 2022

GeoffreyBooth left a comment

JakobJingleheimer commented Apr 27, 2022 •

edited

Loading

JakobJingleheimer commented Apr 27, 2022

aduh95 commented Apr 27, 2022

JakobJingleheimer left a comment

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented Apr 28, 2022

nodejs-github-bot commented Apr 29, 2022

esm: fix imports from non-file module #42881

esm: fix imports from non-file module #42881

Conversation

aduh95 commented Apr 27, 2022

nodejs-github-bot commented Apr 27, 2022

JakobJingleheimer Apr 27, 2022

Choose a reason for hiding this comment

JakobJingleheimer Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

aduh95 Apr 27, 2022

Choose a reason for hiding this comment

GeoffreyBooth left a comment

Choose a reason for hiding this comment

JakobJingleheimer commented Apr 27, 2022 • edited Loading

JakobJingleheimer commented Apr 27, 2022

aduh95 commented Apr 27, 2022

JakobJingleheimer left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented Apr 28, 2022

nodejs-github-bot commented Apr 29, 2022

JakobJingleheimer Apr 27, 2022 •

edited

Loading

JakobJingleheimer commented Apr 27, 2022 •

edited

Loading