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.

Sign up for GitHub

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

Jump to bottom

cache module with same version and same registry #3289

Closed
wants to merge 1 commit into from
Closed

cache module with same version and same registry #3289

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions lib/module.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ const path = require('path');
const internalModuleReadFile = process.binding('fs').internalModuleReadFile;
const internalModuleStat = process.binding('fs').internalModuleStat;


var moduleCache = {};
// If obj.hasOwnProperty has been overridden, then calling
// obj.hasOwnProperty(prop) will break.
// See: https://github.com/joyent/node/issues/1707
Expand Down Expand Up @@ -76,7 +76,7 @@ function readPackage(requestPath) {
}

try {
var pkg = packageMainCache[requestPath] = JSON.parse(json).main;
var pkg = packageMainCache[requestPath] = JSON.parse(json);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个会导致内存中带有很多无关信息。

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

相比于多次加载模块使用的内存少多了

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use English please?

} catch (e) {
e.path = jsonPath;
e.message = 'Error parsing ' + jsonPath + ': ' + e.message;
Expand All @@ -88,9 +88,18 @@ function readPackage(requestPath) {
function tryPackage(requestPath, exts) {
var pkg = readPackage(requestPath);

if (!pkg) return false;
if (!(pkg && pkg.main)) return false;

var resolved = pkg['_resolved'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does the _resolved property come from?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

{
  "name": "commander",
  "version": "2.6.0",
  "description": "the complete solution for node.js command-line programs",
  "keywords": [
    "command",
    "option",
    "parser",
    "prompt"
  ],
  "author": {
    "name": "TJ Holowaychuk",
    "email": "[email protected]"
  },
  "license": "MIT",
  "repository": {
    "type": "git",
    "url": "https://github.com/tj/commander.js.git"
  },
  "devDependencies": {
    "should": ">= 0.0.1"
  },
  "scripts": {
    "test": "make test"
  },
  "main": "index",
  "engines": {
    "node": ">= 0.6.x"
  },
  "files": [
    "index.js"
  ],
  "gitHead": "c6807fd154dd3b7ce8756f141f8d3acfcc74be60",
  "bugs": {
    "url": "https://github.com/tj/commander.js/issues"
  },
  "homepage": "https://github.com/tj/commander.js",
  "_id": "[email protected]",
  "_shasum": "9df7e52fb2a0cb0fb89058ee80c3104225f37e1d",
  "_from": "[email protected]",
  "_npmVersion": "2.1.12",
  "_nodeVersion": "0.11.14",
  "_npmUser": {
    "name": "zhiyelee",
    "email": "[email protected]"
  },
  "maintainers": [
    {
      "name": "tjholowaychuk",
      "email": "[email protected]"
    },
    {
      "name": "somekittens",
      "email": "[email protected]"
    },
    {
      "name": "zhiyelee",
      "email": "[email protected]"
    },
    {
      "name": "thethomaseffect",
      "email": "[email protected]"
    }
  ],
  "dist": {
    "shasum": "9df7e52fb2a0cb0fb89058ee80c3104225f37e1d",
    "tarball": "http://registry.npmjs.org/commander/-/commander-2.6.0.tgz"
  },
  "directories": {},
  "_resolved": "http://registry.npmjs.org/commander/-/commander-2.6.0.tgz",
  "readme": "ERROR: No README data found!"
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. What if _resolved is not as unique as this patch assumes? Things will break badly, won't they?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or replace to shasum?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean the _shasum field? Same issue: what if it's not unique, e.g., because someone accidentally copy-pasted it from one package.json to another?

You could perhaps take the SHA-1 checksum of the whole package.json file but that requires that node is compiled with openssl support. You can add workarounds for the non-openssl case but I don't know, that gets complex awfully fast.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when running npm install , does it not download the source code from the _resolved url ? so the _resolved url got be unique, otherwise things will break badly, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_resolved is private property from npm registry, should not use it.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or tarball

main = pkg.main;

var filename;
if(resolved) {
filename = moduleCache[resolved] || (moduleCache[resolved] = path.resolve(requestPath, main));
}else{
filename = path.resolve(requestPath, main);
}

var filename = path.resolve(requestPath, pkg);
return tryFile(filename) || tryExtensions(filename, exts) ||
tryExtensions(path.resolve(filename, 'index'), exts);
}
Expand Down