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

feat!: return ResolveError:Builtin("node:{specifier}") from package imports and exports #165

Merged
merged 1 commit into from
May 27, 2024

Conversation

Dunqing
Copy link
Member

@Dunqing Dunqing commented May 24, 2024

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 must be prefixed by node:

Copy link

codspeed-hq bot commented May 24, 2024

CodSpeed Performance Report

Merging #165 will not alter performance

Comparing fix/builtins-with-imports-field (056e7b2) with main (99cb3dd)

Summary

✅ 2 untouched benchmarks

Copy link

codecov bot commented May 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.89%. Comparing base (3ba7638) to head (056e7b2).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #165      +/-   ##
==========================================
+ Coverage   96.84%   96.89%   +0.04%     
==========================================
  Files          11       11              
  Lines        2285     2283       -2     
==========================================
- Hits         2213     2212       -1     
+ Misses         72       71       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Boshen Boshen self-requested a review May 25, 2024 00:00
@Boshen Boshen marked this pull request as draft May 25, 2024 00:00
@Boshen Boshen force-pushed the fix/builtins-with-imports-field branch from bebe30d to 1585f6e Compare May 27, 2024 13:56
@Boshen Boshen changed the title fix: if the specifier is defined in imports field and it resolved result is a builtin module should return ResolvedError:Builtin feat!: return ResolveError:Builtin("node:{specifier}") from package imports and exports 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 Boshen force-pushed the fix/builtins-with-imports-field branch from 1585f6e to 056e7b2 Compare May 27, 2024 14:06
@Boshen Boshen marked this pull request as ready for review May 27, 2024 14:07
@Boshen Boshen merged commit e1713c5 into main May 27, 2024
21 checks passed
@Boshen Boshen deleted the fix/builtins-with-imports-field branch May 27, 2024 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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