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

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

Merged
merged 5 commits into from
Jan 31, 2024

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Sep 6, 2023

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
Copy link
Collaborator

Review requested:

  • @nodejs/loaders

@nodejs-github-bot 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
@GeoffreyBooth
Copy link
Member

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
Copy link
Contributor Author

aduh95 commented Sep 6, 2023

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
Copy link
Contributor Author

aduh95 commented Sep 6, 2023

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
Copy link
Member

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
Copy link
Contributor Author

aduh95 commented Sep 6, 2023

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

@GeoffreyBooth
Copy link
Member

GeoffreyBooth commented Sep 6, 2023

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
Copy link
Contributor Author

aduh95 commented Sep 6, 2023

What would it resolve to?

@aduh95
Copy link
Contributor Author

aduh95 commented Sep 6, 2023

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
Copy link
Member

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
Copy link
Contributor Author

aduh95 commented Sep 6, 2023

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
Copy link
Member

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 Outdated Show resolved Hide resolved
Comment on lines 2988 to 2989
is unsupported. Calling `import.meta.resolve` with a relative URL from a `data:`
module is unsupported.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

aduh95 commented Sep 6, 2023

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
Copy link
Contributor Author

aduh95 commented Sep 7, 2023

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 Outdated Show resolved Hide resolved
@aduh95 aduh95 force-pushed the import-meta-resolve-from-data-url branch from 83a253e to 9de6aa6 Compare October 6, 2023 14:59
@@ -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.',
Copy link
Member

@GeoffreyBooth GeoffreyBooth Oct 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@aduh95 aduh95 Jan 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Show resolved Hide resolved
lib/internal/errors.js Outdated Show resolved Hide resolved
doc/api/errors.md Outdated Show resolved Hide resolved
doc/api/errors.md Outdated Show resolved Hide resolved
@aduh95 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 github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 29, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 force-pushed the import-meta-resolve-from-data-url branch from 201ac05 to ac5dab9 Compare January 31, 2024 13:46
@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 31, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 31, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 merged commit 3fbe157 into nodejs:main Jan 31, 2024
52 checks passed
@aduh95
Copy link
Contributor Author

aduh95 commented Jan 31, 2024

Landed in 3fbe157

rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Feb 9, 2024
PR-URL: nodejs#49516
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Feb 15, 2024
PR-URL: #49516
Reviewed-By: James M Snell <jasnell@gmail.com>
marco-ippolito pushed a commit to marco-ippolito/node that referenced this pull request Feb 19, 2024
PR-URL: nodejs#49516
Reviewed-By: James M Snell <jasnell@gmail.com>
@marco-ippolito marco-ippolito mentioned this pull request Mar 1, 2024
richardlau pushed a commit that referenced this pull request Mar 25, 2024
PR-URL: #49516
Reviewed-By: James M Snell <jasnell@gmail.com>
richardlau pushed a commit that referenced this pull request Mar 25, 2024
PR-URL: #49516
Reviewed-By: James M Snell <jasnell@gmail.com>
@richardlau richardlau mentioned this pull request Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants