-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
Conversation
Review requested:
|
Thank you for looking into this. This is definitely something that we should improve.
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? |
The Safari one does not.
|
If the import map maps |
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, |
I don't understand what do you mean. Could you lay down what would be the expected behavior vs the actual behavior? |
For this code: import('data:text/javascript,export default import.meta.resolve("pkg")').catch(console.error) Currently this code always errors. Assuming that |
What would it resolve to? |
You're talking about the file URL, but we're in a data URL. How could I link the two? |
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 If the above works, then we’re already resolving that |
It doesn't work, and it cannot work. It's documented in Lines 208 to 213 in e11c7b7
|
Well it cannot work as we’ve currently defined I guess the alternative would be to define the Anyway thanks for improving the error messaging, that’s a good improvement for now. |
doc/api/errors.md
Outdated
is unsupported. Calling `import.meta.resolve` with a relative URL from a `data:` | ||
module is unsupported. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
The
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 // /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 |
I don't really like the name of the error ( |
83a253e
to
9de6aa6
Compare
lib/internal/errors.js
Outdated
@@ -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.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
201ac05
to
ac5dab9
Compare
Landed in 3fbe157 |
PR-URL: nodejs#49516 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #49516 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#49516 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #49516 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #49516 Reviewed-By: James M Snell <jasnell@gmail.com>
Not sure about the error code (
ERR_UNSUPPORTED_RESOLVE_REQUEST
). For reference, here are what error message other runtimes emit when doingimport('data:text/javascript,export default import.meta.resolve("bare-package-name")').catch(console.error)
:TypeError: Failed to execute 'resolve' on 'import.meta': Failed to resolve module specifier bare-package-name: Relative references must start with either "/", "./", or "../".
TypeError: Module name, 'bare-package-name' does not resolve to a valid URL.
TypeError: The specifier “bare-package-name” was a bare specifier, but was not remapped to anything. Relative module specifiers must start with “./”, “../” or “/”.
TypeError: Relative import path "bare-package-name" not prefixed with / or ./ or ../
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)
:TypeError: Failed to execute 'resolve' on 'import.meta': Failed to resolve module specifier ./relative: Invalid relative url or base scheme isn't hierarchical.
TypeError: Module name, './relative' does not resolve to a valid URL
TypeError: Error resolving module specifier “./relative”.
TypeError: invalid URL: relative URL with a cannot-be-a-base base
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.