search

npm / normalize-package-data

normalizes package metadata, typically found in package.json file.
Other
196 stars 48 forks source link

deps: replace `is-core-module` with node builtin #224

Closed SuperchupuDev closed 4 months ago

SuperchupuDev commented 4 months ago

This PR replaces usage of is-core-module with node's built-in module.builtinModules, which was added in node 9.3.0, 8.10.0, and 6.13.0. My plan was to use module.isBuiltin but that would be a breaking change as it got added in node 16.17.0 and 18.6.0. Maybe once older node versions get dropped :P. This makes normalize-package-data use 3 less dependencies, bringing down the total count to 8. The only difference this could have is that builtinModules do not include node: builtins, but it doesn't affect this library as names prefixed by node: already throw as they are not valid names.

Packages such as is-core-module's dependencies are only there for support with ancient node versions (>=0.4) and as such have many polyfills, including one that literally emulates hasOwnProperty, and overall end up cluttering the whole javascript ecosystem. Since this package includes an engines field of ^16.14.0 || >=18.0.0, there's no real reason in using it over a built-in. is-core-module in particular does have its own use case (checking the list of built-ins of a certain node version other than the current one), but its use in this library can be replicated by module.builtinModules

References

https://nodejs.org/api/module.html#modulebuiltinmodules

jozefizso commented 4 months ago

This is a great change to switch to native code available in the Node.

wraithgar commented 4 months ago

The only difference this could have is that builtinModules do not include node: builtins, but it doesn't affect this library as names prefixed by node: already throw as they are not valid names.

Yep, this comes after we've validated that : isn't present in the name. This reality was one of the reasons the node: prefix was able to even happen.

This looks good. If you don't mind can you change your require to use the node: prefix?