esm: improve error when calling `import.meta.resolve` from `data:` URL #49516

aduh95 · 2023-09-06T13:30:37Z

Not sure about the error code (ERR_UNSUPPORTED_RESOLVE_REQUEST). For reference, here are what error message other runtimes emit when doing import('data:text/javascript,export default import.meta.resolve("bare-package-name")').catch(console.error):

Chromium: TypeError: Failed to execute 'resolve' on 'import.meta': Failed to resolve module specifier bare-package-name: Relative references must start with either "/", "./", or "../".
Safari: TypeError: Module name, 'bare-package-name' does not resolve to a valid URL.
Firefox: TypeError: The specifier “bare-package-name” was a bare specifier, but was not remapped to anything. Relative module specifiers must start with “./”, “../” or “/”.
Deno: TypeError: Relative import path "bare-package-name" not prefixed with / or ./ or ../
Node.js with this PR: TypeError [ERR_UNSUPPORTED_RESOLVE_REQUEST]: Failed to resolve module specifier "bare-package-name" from "data:text/javascript,export default import.meta.resolve("bare-package-name")": Invalid relative url or base scheme is not hierarchical.

Here are the error messages for import('data:text/javascript,export default import.meta.resolve("./relative")').catch(console.error):

Chromium: TypeError: Failed to execute 'resolve' on 'import.meta': Failed to resolve module specifier ./relative: Invalid relative url or base scheme isn't hierarchical.
Safari: TypeError: Module name, './relative' does not resolve to a valid URL
Firefox: TypeError: Error resolving module specifier “./relative”.
Deno: TypeError: invalid URL: relative URL with a cannot-be-a-base base
Node.js with this PR: TypeError [ERR_UNSUPPORTED_RESOLVE_REQUEST]: Failed to resolve module specifier "./relative" from "data:text/javascript,export default import.meta.resolve("./relative")": Invalid relative url or base scheme is not hierarchical.

nodejs-github-bot · 2023-09-06T13:30:42Z

Review requested:

@nodejs/loaders

lib/internal/modules/esm/resolve.js

GeoffreyBooth · 2023-09-06T16:23:04Z

Thank you for looking into this. This is definitely something that we should improve.

For reference, here are what error message other runtimes emit when doing import('data:text/javascript,export default import.meta.resolve("bare-package-name")').catch(console.error):

All of the other runtime error messages you cite mention relative URLs. I think a better comparison would be what messages they throw for your example code when an import map is registered that defines that bare import. Do any of them resolve it?

aduh95 · 2023-09-06T16:26:26Z

All of the other runtime error messages you cite mention relative URLs.

The Safari one does not.

I think a better comparison would be what messages they throw for your example code when an import map is registered that defines that bare import. Do any of them resolve it?

~~Import maps are only supported by Chromium and Deno, and yes they do resolve them.~~ oops my testing was flawed, they do support it, let me recheck

aduh95 · 2023-09-06T16:34:09Z

If the import map maps bare-package-name to an absolute URL, it resolves it. If the import map maps it to a relative URL, it throws the same error as before.

GeoffreyBooth · 2023-09-06T16:35:32Z

If the import map maps bare-package-name to an absolute URL, it resolves it.

Then I think we should do the same, per our own resolution algorithm. That was my expectation when I tried that code; it doesn’t really make sense to me as a user that it should error. Like, import of a bare specifier within a data: URL works, so therefore so should import.meta.resolve.

aduh95 · 2023-09-06T16:36:59Z

I don't understand what do you mean. Could you lay down what would be the expected behavior vs the actual behavior?

GeoffreyBooth · 2023-09-06T16:41:40Z

For this code:

import('data:text/javascript,export default import.meta.resolve("pkg")').catch(console.error)

Currently this code always errors. Assuming that import('pkg') would resolve, then the above code should no longer error, and import.meta.resolve("pkg") should return the file URL to 'pkg'.

aduh95 · 2023-09-06T16:43:01Z

What would it resolve to?

aduh95 · 2023-09-06T16:44:22Z

should return the file URL to 'pkg'.

You're talking about the file URL, but we're in a data URL. How could I link the two?

GeoffreyBooth · 2023-09-06T16:46:15Z

What would it resolve to?

I’m not sure, but as a user I expect it to work. Like I assume this works?

import('data:text/javascript,import("pkg")')

(Assuming that pkg itself exists, that import('pkg') would work in normal non-data: URL code.)

If the above works, then we’re already resolving that import somehow, so import.meta.resolve would use the same approach.

aduh95 · 2023-09-06T16:50:43Z

It doesn't work, and it cannot work. It's documented in

node/doc/api/esm.md

Lines 208 to 213 in e11c7b7

    
           `data:` URLs only resolve [bare specifiers][Terminology] for builtin modules 
        
           and [absolute specifiers][Terminology]. Resolving 
        
           [relative specifiers][Terminology] does not work because `data:` is not a 
        
           [special scheme][]. For example, attempting to load `./foo` 
        
           from `data:text/javascript,import "./foo";` fails to resolve because there 
        
           is no concept of relative resolution for `data:` URLs.

GeoffreyBooth · 2023-09-06T16:58:18Z

It doesn’t work, and it cannot work.

Well it cannot work as we’ve currently defined data: URLs, but it can work as we can see browsers doing it via import maps. And maybe that’s the answer for us too, that the current filesystem-based resolution algorithm cannot resolve imports in data: URLs but when we add support for import maps then this begins to work when an import map is loaded.

I guess the alternative would be to define the parentURL for data: URLs to be pathToFileURL(process.cwd()), but I’m sure you’ll tell me why that’s a bad idea 😄

Anyway thanks for improving the error messaging, that’s a good improvement for now.

lib/internal/errors.js

GeoffreyBooth · 2023-09-06T17:04:16Z

doc/api/errors.md

+is unsupported. Calling `import.meta.resolve` with a relative URL from a `data:`
+module is unsupported.


Suggested change

is unsupported. Calling `import.meta.resolve` with a relative URL from a `data:`

module is unsupported.

is unsupported. Calling `import.meta.resolve` with a relative URL or bare specifier

from a `data:` module is unsupported.

Right?

It's already covered by the previous sentence: Calling import.meta.resolve with a bare specifier outside of a file: module is unsupported.

I find this paragraph very confusing. Maybe it would be clearer if we made it affirmative instead?

From modules loaded from file: URLs, import.meta.resolve can resolve absolute URLs, relative URLs, and bare specifiers.

From modules loaded from https: URLs, import.meta.resolve can resolve absolute URLs and relative URLs.

From modules loaded from data: URLs, import.meta.resolve can resolve only absolute URLs.

Or whatever the correct support matrix is.

aduh95 · 2023-09-06T17:06:26Z

I guess the alternative would be to define the parentURL for data: URLs to be pathToFileURL(process.cwd()), but I’m sure you’ll tell me why that’s a bad idea 😄

The parentURL for data: modules is whatever module loaded that data: module, so changing it wouldn't help – and yes it would break things.

Well it cannot work as we’ve currently defined data: URLs, but it can work as we can see browsers doing it via import maps. And maybe that’s the answer for us too, that the current filesystem-based resolution algorithm cannot resolve imports in data: URLs but when we add support for import maps then this begins to work when an import map is loaded.

Not really, because we don't have import map support nor anything that looks like it. Worth noting that the same data: URL can be imported from different context (different folder, but even different protocol, e.g. the same data: module can be imported by an HTTP module and a file one). Consider the following:

// /root/file.mjs
export {default} from 'data:text/javascript,export {default} from "pkg"';

// /root/node_modules/pkg/index.cjs
module.exports=1;

// /home/user/file.js
export {default} from 'data:text/javascript,export {default} from "pkg"';

// /home/user/node_modules/pkg/index.cjs
module.exports=2;

and then you run node --input-type=module -e 'import One from 'file:///root/file.js;import OneAsWell from 'file:///home/user/file.js', it would be very confusing.

aduh95 · 2023-09-07T22:50:35Z

I don't really like the name of the error (ERR_UNSUPPORTED_RESOLVE_REQUEST), nor am I happy with the documentation text. Open to suggestions.

doc/api/errors.md

GeoffreyBooth · 2023-10-28T16:57:32Z

lib/internal/errors.js

@@ -1809,6 +1809,9 @@ E('ERR_UNSUPPORTED_ESM_URL_SCHEME', (url, supported) => {
  msg += `. Received protocol '${url.protocol}'`;
  return msg;
 }, Error);
+E('ERR_UNSUPPORTED_RESOLVE_REQUEST',
+  'Failed to resolve module specifier "%s" from "%s": Invalid relative url or base scheme is not hierarchical.',


Suggested change

'Failed to resolve module specifier "%s" from "%s": Invalid relative url or base scheme is not hierarchical.',

'Failed to resolve module specifier "%s" from "%s": Invalid relative URL or unsupported protocol.',

What does “base scheme is not hierarchical” mean?

Maybe when the protocol is data: just say: import.meta.resolve is not supported within data: URLs ?

It's not just with data: URLs, however. It's really any non-special URL whose hierarchy semantics cannot be determined. For instance, suppose we supported a hypothetical fake:module URL in the future, this error would also apply in that case since new URL('something', 'fake:module') would end up as an invalid URL error.

One way we could improve the error message is to say something like:

Failed to resolve module specifier "...": import.meta.resolve is not supported with ${baseUrl}

Where ${baseUrl} is the full url of the referring module.

But import.meta.resolve is supported with data: URLs, just it won't accept relative URLs / bare specifiers. I inspired myself for this error message from Chromium's error message, which might not be perfect, but is probably at least technically correct.

doc/api/errors.md

lib/internal/errors.js

doc/api/errors.md

nodejs-github-bot · 2024-01-29T14:53:34Z

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

nodejs-github-bot · 2024-01-29T18:01:37Z

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

nodejs-github-bot · 2024-01-29T21:27:01Z

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

nodejs-github-bot · 2024-01-31T13:30:37Z

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

nodejs-github-bot · 2024-01-31T13:50:56Z

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

nodejs-github-bot · 2024-01-31T19:01:17Z

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

aduh95 · 2024-01-31T22:22:23Z

Landed in 3fbe157

PR-URL: nodejs#49516 Reviewed-By: James M Snell <[email protected]>

PR-URL: #49516 Reviewed-By: James M Snell <[email protected]>

PR-URL: nodejs#49516 Reviewed-By: James M Snell <[email protected]>

PR-URL: #49516 Reviewed-By: James M Snell <[email protected]>

nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. labels Sep 6, 2023

anonrig reviewed Sep 6, 2023

View reviewed changes

lib/internal/modules/esm/resolve.js Outdated Show resolved Hide resolved

GeoffreyBooth reviewed Sep 6, 2023

View reviewed changes

GeoffreyBooth reviewed Sep 7, 2023

View reviewed changes

doc/api/errors.md Outdated Show resolved Hide resolved

aduh95 force-pushed the import-meta-resolve-from-data-url branch from 83a253e to 9de6aa6 Compare October 6, 2023 14:59

jasnell reviewed Oct 28, 2023

View reviewed changes

doc/api/errors.md Show resolved Hide resolved

jasnell approved these changes Oct 28, 2023

View reviewed changes

GeoffreyBooth reviewed Oct 28, 2023

View reviewed changes

aduh95 commented Jan 29, 2024

View reviewed changes

lib/internal/errors.js Outdated Show resolved Hide resolved

aduh95 commented Jan 29, 2024

View reviewed changes

doc/api/errors.md Outdated Show resolved Hide resolved

aduh95 commented Jan 29, 2024

View reviewed changes

doc/api/errors.md Outdated Show resolved Hide resolved

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 Jan 29, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 29, 2024

aduh95 added 5 commits January 31, 2024 14:45

esm: improve error when calling import.meta.resolve from data: URL

f2516bb

fix lint

8c94c0e

Update lib/internal/errors.js

6aa3de7

Apply suggestions from code review

24afb0b

Update doc/api/errors.md

ac5dab9

aduh95 force-pushed the import-meta-resolve-from-data-url branch from 201ac05 to ac5dab9 Compare January 31, 2024 13:46

aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 31, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 31, 2024

aduh95 merged commit 3fbe157 into nodejs:main Jan 31, 2024
52 checks passed

rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Feb 9, 2024

esm: improve error when calling import.meta.resolve from data: URL

a57cc41

PR-URL: nodejs#49516 Reviewed-By: James M Snell <[email protected]>

targos pushed a commit that referenced this pull request Feb 15, 2024

esm: improve error when calling import.meta.resolve from data: URL

08832e7

PR-URL: #49516 Reviewed-By: James M Snell <[email protected]>

marco-ippolito pushed a commit to marco-ippolito/node that referenced this pull request Feb 19, 2024

esm: improve error when calling import.meta.resolve from data: URL

0041ca5

PR-URL: nodejs#49516 Reviewed-By: James M Snell <[email protected]>

marco-ippolito mentioned this pull request Mar 1, 2024

v21.7.0 proposal #51932

Merged

richardlau pushed a commit that referenced this pull request Mar 25, 2024

esm: improve error when calling import.meta.resolve from data: URL

db03425

PR-URL: #49516 Reviewed-By: James M Snell <[email protected]>

richardlau pushed a commit that referenced this pull request Mar 25, 2024

esm: improve error when calling import.meta.resolve from data: URL

a6b3569

PR-URL: #49516 Reviewed-By: James M Snell <[email protected]>

richardlau mentioned this pull request Mar 25, 2024

v20.12.0 proposal #52212

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

esm: improve error when calling `import.meta.resolve` from `data:` URL #49516

esm: improve error when calling `import.meta.resolve` from `data:` URL #49516

aduh95 commented Sep 6, 2023 •

edited

Loading

nodejs-github-bot commented Sep 6, 2023

GeoffreyBooth commented Sep 6, 2023

aduh95 commented Sep 6, 2023 •

edited

Loading

aduh95 commented Sep 6, 2023

GeoffreyBooth commented Sep 6, 2023

aduh95 commented Sep 6, 2023 •

edited

Loading

GeoffreyBooth commented Sep 6, 2023 •

edited

Loading

aduh95 commented Sep 6, 2023

aduh95 commented Sep 6, 2023

GeoffreyBooth commented Sep 6, 2023

aduh95 commented Sep 6, 2023

GeoffreyBooth commented Sep 6, 2023

GeoffreyBooth Sep 6, 2023

aduh95 Sep 6, 2023

GeoffreyBooth Sep 6, 2023

aduh95 commented Sep 6, 2023 •

edited

Loading

aduh95 commented Sep 7, 2023

GeoffreyBooth Oct 28, 2023 •

edited

Loading

jasnell Oct 28, 2023

aduh95 Jan 29, 2024 •

edited

Loading

nodejs-github-bot commented Jan 29, 2024

nodejs-github-bot commented Jan 29, 2024

nodejs-github-bot commented Jan 29, 2024

nodejs-github-bot commented Jan 31, 2024

nodejs-github-bot commented Jan 31, 2024

nodejs-github-bot commented Jan 31, 2024

aduh95 commented Jan 31, 2024

		is unsupported. Calling `import.meta.resolve` with a relative URL from a `data:`
		module is unsupported.

	'Failed to resolve module specifier "%s" from "%s": Invalid relative url or base scheme is not hierarchical.',
	'Failed to resolve module specifier "%s" from "%s": Invalid relative URL or unsupported protocol.',

esm: improve error when calling import.meta.resolve from data: URL #49516

esm: improve error when calling import.meta.resolve from data: URL #49516

Conversation

aduh95 commented Sep 6, 2023 • edited Loading

nodejs-github-bot commented Sep 6, 2023

GeoffreyBooth commented Sep 6, 2023

aduh95 commented Sep 6, 2023 • edited Loading

aduh95 commented Sep 6, 2023

GeoffreyBooth commented Sep 6, 2023

aduh95 commented Sep 6, 2023 • edited Loading

GeoffreyBooth commented Sep 6, 2023 • edited Loading

aduh95 commented Sep 6, 2023

aduh95 commented Sep 6, 2023

GeoffreyBooth commented Sep 6, 2023

aduh95 commented Sep 6, 2023

GeoffreyBooth commented Sep 6, 2023

GeoffreyBooth Sep 6, 2023

Choose a reason for hiding this comment

aduh95 Sep 6, 2023

Choose a reason for hiding this comment

GeoffreyBooth Sep 6, 2023

Choose a reason for hiding this comment

aduh95 commented Sep 6, 2023 • edited Loading

aduh95 commented Sep 7, 2023

GeoffreyBooth Oct 28, 2023 • edited Loading

Choose a reason for hiding this comment

jasnell Oct 28, 2023

Choose a reason for hiding this comment

aduh95 Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

nodejs-github-bot commented Jan 29, 2024

nodejs-github-bot commented Jan 29, 2024

nodejs-github-bot commented Jan 29, 2024

nodejs-github-bot commented Jan 31, 2024

nodejs-github-bot commented Jan 31, 2024

nodejs-github-bot commented Jan 31, 2024

aduh95 commented Jan 31, 2024

esm: improve error when calling `import.meta.resolve` from `data:` URL #49516

esm: improve error when calling `import.meta.resolve` from `data:` URL #49516

aduh95 commented Sep 6, 2023 •

edited

Loading

aduh95 commented Sep 6, 2023 •

edited

Loading

aduh95 commented Sep 6, 2023 •

edited

Loading

GeoffreyBooth commented Sep 6, 2023 •

edited

Loading

aduh95 commented Sep 6, 2023 •

edited

Loading

GeoffreyBooth Oct 28, 2023 •

edited

Loading

aduh95 Jan 29, 2024 •

edited

Loading