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

Unexpected result NotFound('node:http') with imports field and builtins: true option #164

Closed
hyf0 opened this issue May 24, 2024 · 3 comments · Fixed by #165
Closed

Unexpected result NotFound('node:http') with imports field and builtins: true option #164

hyf0 opened this issue May 24, 2024 · 3 comments · Fixed by #165

Comments

@hyf0
Copy link
Contributor

hyf0 commented May 24, 2024

import fs from '#fs'
import http from '#http'
fs.readFileSync()
http.createServer()
{
  "imports": {
    "#fs": {
      "node": "fs",
      "default": "./empty.js"
    },
    "#http": {
      "node": "node:http",
      "default": "./empty.js"
    }
  }
}

While resolving #fs, oxc-resolver correctly maps #fs to fs. Then oxc-resolver returns NotFound("fs"), which is also correct with builtins: false.

However, with builtin: true, oxc-resolver should return Builtin('fs') or Builtins('node:http').

@hyf0
Copy link
Contributor Author

hyf0 commented May 24, 2024

Rolldown currently will handles Builtin(xxx) specially, which is treating xxx as a valid external module while targeting node platform. But the behavior said above makes rolldown can't handle builtin node modules right due to that oxc-resolver return NotFound.

Rolldown treats NotFound as a invalid case and will throw a warning. So it will be better to make oxc-resolver also return Builtin for the above described behavior to be more consistency for resolving builtin node modules with builtin: true.

@ematipico
Copy link
Collaborator

However, with builtin: true, oxc-resolver should return Builtin('fs') or Builtins('node:http').

I don't understand one thing though: what's the issue? Why should it return this result? What does it return instead?

@hyf0
Copy link
Contributor Author

hyf0 commented May 25, 2024

However, with builtin: true, oxc-resolver should return Builtin('fs') or Builtins('node:http').

I don't understand one thing though: what's the issue? Why should it return this result? What does it return instead?

For resolving #http with ResolveOptions#builtin: true, oxc-resolver should return Err(ResolveError::Builtin('node:http')) instead of Err(ResolveError::NotFound).

Boshen pushed a commit that referenced this issue May 27, 2024
… imports and exports

closes #164

According to the ESM specification https://nodejs.org/api/esm.html#resolution-and-loading-algorithm

```
// PACKAGE_RESOLVE(packageSpecifier, parentURL)
// 3. If packageSpecifier is a Node.js builtin module name, then
//   1. Return the string "node:" concatenated with packageSpecifier.
```

The returned value should be prefixed by `node:`
Boshen pushed a commit that referenced this issue May 27, 2024
… imports and exports

closes #164

According to the ESM specification https://nodejs.org/api/esm.html#resolution-and-loading-algorithm

```
// PACKAGE_RESOLVE(packageSpecifier, parentURL)
// 3. If packageSpecifier is a Node.js builtin module name, then
//   1. Return the string "node:" concatenated with packageSpecifier.
```

The returned value should be prefixed by `node:`
Boshen pushed a commit that referenced this issue May 27, 2024
… imports and exports (#165)

closes #164

According to the ESM specification https://nodejs.org/api/esm.html#resolution-and-loading-algorithm

```
// PACKAGE_RESOLVE(packageSpecifier, parentURL)
// 3. If packageSpecifier is a Node.js builtin module name, then
//   1. Return the string "node:" concatenated with packageSpecifier.
```

The returned value should be prefixed by `node:`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants