esm: fix imports from non-file module by aduh95 · Pull Request #42881 · nodejs/node · GitHub

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

Review requested:

@nodejs/modules

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?

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.

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.

@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).

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.

Sure! That works for me!

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

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 outstanding review comments, 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 pushed a commit that referenced this pull request May 31, 2022

esm: fix imports from non-file module

c6f4a6f

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>

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

		const parentURL = fileURLToPath(parsedParentURL?.href);

esm: fix imports from non-file module by aduh95 · Pull Request #42881 · nodejs/node · GitHub

Uh oh!

Uh oh!

esm: fix imports from non-file module - #42881

esm: fix imports from non-file module#42881nodejs-github-bot merged 3 commits intonodejs:masternodejs/node:masterfrom aduh95:fix-import-from-data-modulesaduh95/node:fix-import-from-data-modulesCopy head branch name to clipboard

Conversation

aduh95 commented Apr 27, 2022

Uh oh!

nodejs-github-bot commented Apr 27, 2022

Uh oh!

JakobJingleheimer Apr 27, 2022

Choose a reason for hiding this comment

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

GeoffreyBooth left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

JakobJingleheimer commented Apr 27, 2022

Uh oh!

aduh95 commented Apr 27, 2022

Uh oh!

JakobJingleheimer left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

nodejs-github-bot commented Apr 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

esm: fix imports from non-file module#42881
nodejs-github-bot merged 3 commits into
nodejs:masterfrom
aduh95:fix-import-from-data-modules

JakobJingleheimer Apr 27, 2022 •
edited
Loading

JakobJingleheimer commented Apr 27, 2022 •
edited
Loading