[1.3-beta.11] Can't depend on the pkginfo npm package or any package that uses it

[aldeed]

Reproduction app: https://github.com/aldeed/13test

To reproduce:

1. Clone the app
2. npm install
3. meteor

The error message:

W20160223-09:09:47.516(-6)? (STDERR) Error: ENOENT, no such file or directory '/node_modules/pkginfo/lib'
W20160223-09:09:47.517(-6)? (STDERR)     at Object.fs.readdirSync (fs.js:666:18)
W20160223-09:09:47.517(-6)? (STDERR)     at Function.pkginfo.find (node_modules/pkginfo/lib/pkginfo.js:95:1)
W20160223-09:09:47.517(-6)? (STDERR)     at Function.pkginfo.read (node_modules/pkginfo/lib/pkginfo.js:119:1)
W20160223-09:09:47.517(-6)? (STDERR)     at module.exports (node_modules/pkginfo/lib/pkginfo.js:69:1)
W20160223-09:09:47.517(-6)? (STDERR)     at meteorInstall.node_modules.pkginfo.lib.pkginfo.js (node_modules/pkginfo/lib/pkginfo.js:132:1)
W20160223-09:09:47.517(-6)? (STDERR)     at fileEvaluate (packages/modules-runtime/.npm/package/node_modules/install/install.js:202:1)
W20160223-09:09:47.518(-6)? (STDERR)     at require (packages/modules-runtime/.npm/package/node_modules/install/install.js:75:1)
W20160223-09:09:47.518(-6)? (STDERR)     at meteorInstall.server.main.js (/Users/eric/DevProjects/13test/.meteor/local/build/programs/server/app/app.js:9:1)
W20160223-09:09:47.518(-6)? (STDERR)     at fileEvaluate (packages/modules-runtime/.npm/package/node_modules/install/install.js:202:1)
W20160223-09:09:47.518(-6)? (STDERR)     at require (packages/modules-runtime/.npm/package/node_modules/install/install.js:75:1)

This error happens simply by doing import 'pkginfo' somewhere in the app. It also happens with any other package that depends on pkginfo, which seems to be many. I initially saw this when importing logstar, which imports winston, which imports pkginfo.

I don't know enough details about how the npm imports work in 1.3, but it seems maybe related to the fact that npm packages used by Meteor packages are not in the same directory as npm packages used by the app.

[benjamn]

Apologies in advance for the wall of text. This issue provides an excellent example of a category of npm packages that are difficult (but not impossible!) to use outside of Node, so I'm going to explain all the background (and some potential solutions) in as much detail as I can. Don't feel obligated to read or respond to all of this; the tl;dr is that we're working on the challenges posed by packages like pkginfo, and we should have a solution in 1.3.1 if not in time for 1.3.

So, what exactly is going on here?

The pkginfo package attempts to find and parse the current package.json file. In order to do that, it assumes that module.id is a real, absolute file system path that can be read by fs.readdirSync. That happens to be the case in Node, at least most of the time, but CommonJS only requires that require(module.id) returns the current module.exports object, and leaves unspecified the question of how module identifiers map to file system paths.

In other words, pkginfo may work in a Node environment, but it is not portable to other CommonJS environments. If it aspired to be portable, it would treat module.id as a module identifier and use require to load package.json files from parent "directories" (in quotes because module identifiers look like /-delimited paths but are not necessarily file system paths). The advantage of this approach is that it would work on the client as well as the server, not to mention in Meteor.

Why can't Meteor expose real, absolute file system paths via module.id or path.join(__dirname, __filename)? The short answer is that I wanted to give the Meteor module system a structure that made sense in terms of CommonJS require and Node's own module resolution rules, without breaking backwards compatibility with existing Meteor rules about directory structure. For example, a Meteor package can be installed from Atmosphere, or installed locally in a packages/ directory, or installed from a checkout of github.com/meteor/meteor, but you should always be able to import stuff from a package called asdf by doing import ... from "meteor/asdf" or require("meteor/asdf"). This consistency demands that all modules in Meteor packages have module.ids that start with /node_modules/meteor/..., which unfortunately hides the truth about where the package actually lives on your file system, or the file system of the server where your app is deployed.

With all of that background explanation out of the way, what can we do about pkginfo, and packages that depend on it?

• The "right" way would be to pull request pkginfo to rely only on what CommonJS mandates, and not the details of Node's particular implementation of CommonJS, and then pull request all of pkginfo's dependents to update to the new version. Unfortunately, that might take a while.
• A really hacky way would be to monkey-patch fs.readdirSync and friends to return the right thing when given a module.id path. Where would that kind of effort end? I'm not entirely sure.
• Yet another, slightly less hacky, approach: provide an alternate implementation of pkginfo that works within the rules of CommonJS, along the lines of Provide shims for built-in Node libraries on the client, when possible. #6056, though that would probably require adding new features to the Meteor module system to enable globally overriding a specific package name. If the same solution helped with Provide shims for built-in Node libraries on the client, when possible. #6056, that might be the way to go.

Thanks for reading this far, and thanks for putting this issue on our radar!

[aldeed]

Thanks for the explanation @benjamn. I have a potential pull request here that seems to work: indexzero/node-pkginfo@master...aldeed:master

Can you review it? I would like to add one or more tests to pkginfo to prove this works, too, but I'm not quite expert enough in requirejs to replicate what Meteor is doing. Can you let me know what a proper test would be? I noticed that sometimes module.filename is set and sometimes it's undefined, but in both cases it and the module.id start with /node_modules. So I guess we need tests to replicate those conditions.

[benjamn]

Awesome! This PR looks good to me, though I would change this line

throw new Error('Could not find package.json up from /');

to be a little more descriptive:

throw new Error('Could not find package.json up from ' +
                (pmodule.filename || pmodule.id));

(I know you were just trying to keep the original behavior.)