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

Prefer Node's core modules over file modules #60

Merged
merged 1 commit into from
Sep 2, 2018
Merged

Prefer Node's core modules over file modules #60

merged 1 commit into from
Sep 2, 2018

Conversation

ljani
Copy link
Contributor

@ljani ljani commented Aug 29, 2018

This commit makes this resolver behave like Node does:

Core modules are always preferentially loaded if their identifier is passed to require()

Additionally this increases performance by avoiding unnecessary disk access. For example webpack calls require("crypto") in a loop.

This commit fixes #56.

@ljani
Copy link
Contributor Author

ljani commented Aug 29, 2018

Hmm, right, I tested the code in JavaScript without thinking too much. Turns out process.binding has been deprecated lately. Any ideas how to get list of native modules included in the runtime?

I'll close this PR until a better way is found.

@ljani ljani closed this Aug 29, 2018
@ljani ljani reopened this Aug 29, 2018
@ljani
Copy link
Contributor Author

ljani commented Aug 29, 2018

Apparently there is Module.builtinModules, but it's only since v9.3.0. Should we fallback to using the internal API?

@coveralls
Copy link

coveralls commented Aug 29, 2018

Coverage Status

Coverage remained the same at 81.279% when pulling 1dfbf49 on ljani:prefer-core-modules into 6ab6566 on dividab:master.

@Jontem
Copy link
Collaborator

Jontem commented Aug 30, 2018

We could use a hardcoded list when node < 9.3.0?

[ 'assert',
  'buffer',
  'child_process',
  'cluster',
  'crypto',
  'dgram',
  'dns',
  'domain',
  'events',
  'fs',
  'http',
  'https',
  'net',
  'os',
  'path',
  'punycode',
  'querystring',
  'readline',
  'stream',
  'string_decoder',
  'tls',
  'tty',
  'url',
  'util',
  'v8',
  'vm',
  'zlib' ]

@jonaskello
Copy link
Member

Checking Module.builtinModules and then fallback to hardcoded array sounds good to me. @ljani what do you think?

@ljani
Copy link
Contributor Author

ljani commented Aug 30, 2018

Works for me, hold on...

This commit makes this resolver behave like Node does [0]:
"Core modules are always preferentially loaded if their identifier is passed to require()"

Additionally this increases performance by avoiding unnecessary disk access. For example webpack calls require("crypto") in a loop.

This commit fixes #56.

[0]: https://nodejs.org/api/modules.html#modules_core_modules
@Jontem Jontem merged commit 4286b34 into dividab:master Sep 2, 2018
@Jontem
Copy link
Collaborator

Jontem commented Sep 2, 2018

@ljani Thanks!

@ljani ljani deleted the prefer-core-modules branch September 2, 2018 19:11
@ljani
Copy link
Contributor Author

ljani commented Sep 10, 2018

Could this be published to npm? Thanks!

@jonaskello
Copy link
Member

Published in 3.6.0, sorry for the delay :-).

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.

Node core modules are not always prioritized over the others
4 participants