Skip to content

Commit

Permalink
module: speed up package.json parsing
Browse files Browse the repository at this point in the history
If the package.json does not contain the string '"main"', skip parsing
it to JSON.

Note that this changes the behavior of the module loader in the presence
of package.json files that don't contain legal JSON.  Such files used to
throw an exception but now they are simply ignored unless they contain a
"main" property.

To me, that seems like a good trade-off: I observe a 25% reduction in
start-up time on a medium-sized application[0].

[0] https://github.com/strongloop/sls-sample-app

PR-URL: #15767
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
bnoordhuis committed Nov 15, 2017
1 parent 1132ea7 commit fdbb6dd
Show file tree
Hide file tree
Showing 4 changed files with 3 additions and 10 deletions.
3 changes: 3 additions & 0 deletions lib/module.js
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,9 @@ function readPackage(requestPath) {
return false;
}

if (!/"main"/.test(json))
return packageMainCache[requestPath] = undefined;

This comment has been minimized.

Copy link
@jdalton

jdalton Nov 17, 2017

Member

👆 Why set the cache to, and return, undefined instead of returning false
as when json === undefined?

It looks like the only place that calls readPackage(), and stores it in pkg, checks for it as if (!pkg). Should't the cache and return behavior be consistent for the cache misses here?

try {
var pkg = packageMainCache[requestPath] = JSON.parse(json).main;
} catch (e) {
Expand Down
1 change: 0 additions & 1 deletion test/fixtures/packages/invalid/index.js

This file was deleted.

1 change: 0 additions & 1 deletion test/fixtures/packages/invalid/package.json

This file was deleted.

8 changes: 0 additions & 8 deletions test/sequential/test-module-loading.js
Original file line number Diff line number Diff line change
Expand Up @@ -100,14 +100,6 @@ const d2 = require('../fixtures/b/d');
assert.notStrictEqual(threeFolder, three);
}

console.error('test package.json require() loading');
assert.throws(
function() {
require('../fixtures/packages/invalid');
},
/^SyntaxError: Error parsing .+: Unexpected token , in JSON at position 1$/
);

assert.strictEqual(require('../fixtures/packages/index').ok, 'ok',
'Failed loading package');
assert.strictEqual(require('../fixtures/packages/main').ok, 'ok',
Expand Down

0 comments on commit fdbb6dd

Please sign in to comment.