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

Uncaught error with --experimental-import-meta-resolve #55518

Closed
ph-fritsche opened this issue Oct 24, 2024 · 8 comments · Fixed by #55777
Closed

Uncaught error with --experimental-import-meta-resolve #55518

ph-fritsche opened this issue Oct 24, 2024 · 8 comments · Fixed by #55777
Labels
confirmed-bug Issues with confirmed bugs. experimental Issues and PRs related to experimental features.

Comments

@ph-fritsche
Copy link

Version

22.10

Platform

Linux be964f1f5acb 6.10.4-linuxkit #1 SMP PREEMPT_DYNAMIC Wed Oct  2 16:39:54 UTC 2024 x86_64 Linux

https://hub.docker.com/layers/library/node/22-alpine/images/sha256-c5088ba04c8fcce84e45c9ab8767af401424943a8bfd90ede5a68aefc4f42808

Subsystem

No response

What steps will reproduce the bug?

Run with --experimental-import-meta-resolve:

function resolve(specifier, parentURL) {
    console.log(`\n§ ${parentURL} --> ${specifier}`)
    try {
        console.log(`Resolved: ${import.meta.resolve(specifier, parentURL)}`)
    } catch (e) {
        console.log(`Error: ${e.code}`)
    }
}

resolve('foo', 'file:///bar.js')
resolve('./foo.js', 'http://example.com/bar.js')
resolve('file:///foo.js', 'http://example.com/bar.js')
resolve('foo', 'http://example.com/bar.js')

How often does it reproduce? Is there a required condition?

Always reproduces

What is the expected behavior? Why is that the expected behavior?

A catchable error that the specifier can't be resolved. This allows to provide a helpful error message that the user should add a custom resolver/loader.

What do you see instead?

§ file:///bar.js --> foo
Error: ERR_MODULE_NOT_FOUND

§ http://example.com/bar.js --> ./foo.js
Resolved: http://example.com/foo.js

§ http://example.com/bar.js --> file:///foo.js
Error: ERR_INVALID_URL_SCHEME

§ http://example.com/bar.js --> foo

  #  node[1799]: static void node::modules::BindingData::GetPackageScopeConfig(const v8::FunctionCallbackInfo<v8::Value>&) at ../src/node_modules.cc:403
  #  Assertion failed: file_url

----- Native stack trace -----


----- JavaScript stack trace -----

1: getPackageScopeConfig (node:internal/modules/package_json_reader:132:33)
2: packageResolve (node:internal/modules/esm/resolve:792:43)
3: moduleResolve (node:internal/modules/esm/resolve:907:18)
4: defaultResolve (node:internal/modules/esm/resolve:1037:11)
5: defaultResolve (node:internal/modules/esm/loader:650:12)
6: #cachedDefaultResolve (node:internal/modules/esm/loader:599:25)
7: #resolveAndMaybeBlockOnLoaderThread (node:internal/modules/esm/loader:615:38)
8: resolveSync (node:internal/modules/esm/loader:632:52)
9: resolve (node:internal/modules/esm/initialize_import_meta:33:25)
10: resolve (file:///workspaces/nodejs-import-meta-resolve/[eval1]:6:46)


Aborted

Additional information

No response

@avivkeller avivkeller added experimental Issues and PRs related to experimental features. confirmed-bug Issues with confirmed bugs. labels Oct 24, 2024
@avivkeller
Copy link
Member

The assertion is failing because it expects the URL's protocol to be file:, when it is http:

@cedrick-ah
Copy link

I would like to work on this. In the function moduleResolve in lib/internal/modules/esm/resolve.js when the protocol of the parsed parentURL is not file the parentURL is returned. Instead of just returning the parentURL, can we do something like this in the function moduleResolve:

  if (resolved.protocol !== 'file:') {
     throw new ERR_INVALID_URL_SCHEME(resolved.protocol);
  }

@avivkeller
Copy link
Member

avivkeller commented Oct 25, 2024

No, That wouldn't work. Under the hood, the issue is because it's trying to read the package configuration for the URL, which it can't do. There are cases where a URL can be used, this is not one of them.

@cedrick-ah
Copy link

cedrick-ah commented Oct 25, 2024

The assertion is failing because it expects the URL's protocol to be file:, when it is http:

According to this, can we then check for the URL protocol in throwIfInvalidParentURL?

@avivkeller
Copy link
Member

avivkeller commented Oct 25, 2024

I believe @JakobJingleheimer may know more than I (regarding the package.json reading) (if not, sorry for the ping)

@JakobJingleheimer
Copy link
Member

The provided code sample looks like a loader hook. If so, import.meta.resolve() is not available within loaders.

@ph-fritsche
Copy link
Author

The code sample just demonstrates the different behavior for resolving a bare specifier in http: context. For other input there is either a result or a catchable error, but in this case the assertion error crashes the node process.

@avivkeller
Copy link
Member

The provided code sample looks like a loader hook. If so, import.meta.resolve() is not available within loaders.

Regardless, it shouldn't crash at the CPP level. It should probably through a module not found error?

aduh95 pushed a commit that referenced this issue Nov 16, 2024
PR-URL: #55777
Fixes: #55518
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
tpoisseau pushed a commit to tpoisseau/node that referenced this issue Nov 21, 2024
PR-URL: nodejs#55777
Fixes: nodejs#55518
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this issue Nov 26, 2024
PR-URL: nodejs#55777
Fixes: nodejs#55518
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
ruyadorno pushed a commit that referenced this issue Nov 27, 2024
PR-URL: #55777
Fixes: #55518
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
ruyadorno pushed a commit that referenced this issue Nov 27, 2024
PR-URL: #55777
Fixes: #55518
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
ruyadorno pushed a commit that referenced this issue Nov 27, 2024
PR-URL: #55777
Fixes: #55518
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. experimental Issues and PRs related to experimental features.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants