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

Support module.builtinModules #13

Closed
voxpelli opened this issue Dec 18, 2017 · 6 comments
Closed

Support module.builtinModules #13

voxpelli opened this issue Dec 18, 2017 · 6 comments

Comments

@voxpelli
Copy link
Contributor

As Node.js itself since 9.3.0 supports this functionality trhough module.builtinModules it would be neat to have this module use it on those newer versions.

Docs here: https://nodejs.org/dist/latest-v9.x/docs/api/modules.html#modules_module_builtinmodules

Ping @watson

@juliangruber
Copy link
Owner

Am I doing something wrong?

$ node -pe "[process.version, module.builtinModules]"
[ 'v9.3.0', undefined ]

@watson
Copy link
Contributor

watson commented Dec 18, 2017

I thought about that, but if we want to support requesting a list of modules for any node version, I can't see how we can use this feature unfortunately.

We could make a check for if version === process.version && semver.gte(version, '9.3.0') and only use it in that case, but that's kind of silly as we would have to support the same version even if you requested that version specifically, but was for instance running node 8.x.

So the only reason I can see for using module.builtinModules would be if we dropped support for supplying a custom version.

But I might have overlooked something 😄

@watson
Copy link
Contributor

watson commented Dec 18, 2017

@juliangruber here you go:

$ node -pe "[process.version, require('module').builtinModules]"

And you're not alone: nodejs/node#17712

@voxpelli
Copy link
Contributor Author

Right, as it only was used to check against the current version in dependency-check I overlooked that 2.x introduced the possibility to look for builtins in other versions as well.

I can see the use case for that, although the use case in eg. dependency-check would benefit more from it using the core method for 9.3.0 and forward - it would be more future proof and be pretty much guaranteed to be correct from now on.

Perhaps that’s a use case for a new module? Or how would you feel about adding a specific polyfill:ish method for checking against current version? Would you prefer a new wrapper module that uses this for all versions prior to 9.3.0 and the core one for the rest?

I don’t like the idea of this module sometimes returning data from the core function and sometimes not in the same function call, every call to the same function should work the same I think.

@juliangruber
Copy link
Owner

I don’t like the idea of this module sometimes returning data from the core function and sometimes not in the same function call, every call to the same function should work the same I think.

I agree with this. And this module has already developed in multiple directions. Initially it was only used by browserify to check which module to ignore while bundling - and in this case already it's not 100% clear what to do about the different node versions.

With the recent addition of passing an optional node version string, this module opened up to more possibilities while still easily supporting the browserify use case without sacrificing any existing functionality.

In dependency-check I understand that it makes most sense to return exactly the modules supported by the current node version. After all, it's not targeting for a different environment, as browserify most likely does.

What about dependency-check simply using this snippet:

const builtins = require('module').builtinModules || require('builtins')()

We can also advice for this one liner until we can deprecate this module completely.

Otherwise I too vote for creating either a wrapper module, or: setting up a travis cron job that keeps this module always up to date with all supported node versions, generated via some script.

@voxpelli
Copy link
Contributor Author

I'm going to close this, I see the point that everyone has made here and I can agree with that

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

No branches or pull requests

3 participants