chore(node-resolve): remove is-builtin-module #1735
Conversation
|
FYI the engine constraint in here actually specifies 14 or something. Which is why I have used the built in modules list (available in 14), but not Just to be clear If we can bump the engine constraint to 16, we could use the built in function |
Node has shipped `builtinModules` for some time now, so we no longer need a third party package to do this. Once the `engines` constraint is bumped in the `package.json`, we can also move to using the built-in `isBuiltin` function (available since 16.x).
|
any chance we can get this merged? 👀 would be a good to pull it into a few dependents |
| import isBuiltinModule from 'is-builtin-module'; | ||
| import { builtinModules } from 'module'; |
There was a problem hiding this comment.
Won't this cause a behavior change if you're bundling on one version of node, but a newer node has that module available? Or does it not matter because node will never add any new module without the node: prefix?
There was a problem hiding this comment.
It's unlikely they will add any new modules without the prefix as far as I understand
Also the third party package also has a fixed list which won't always be accurate to the runtime node
|
@shellscape any chance you can take a look at this? would be good to close it off |
|
any possibility of a review from someone here? would be super nice to merge this so it doesn't get left behind |
|
|
Node has shipped
builtinModulesfor some time now, so we no longer need a third party package to do this.Once the
enginesconstraint is bumped in thepackage.json, we can also move to using the built-inisBuiltinfunction (available since 16.x).Rollup Plugin Name:
node-resolveThis PR contains:
Are tests included?
Breaking Changes?