If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

let Jetpack use CommonJS and NPM modules

RESOLVED WONTFIX

Status

Add-on SDK
General
P3
normal
RESOLVED WONTFIX
7 years ago
6 years ago

People

(Reporter: warner, Assigned: warner)

Tracking

unspecified
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

7 years ago
It would be great if we could take advantage of modules written to the CommonJS standard, specifically those in the NPM Registry (http://npm.mape.me/). In particular, it would be great if the node.js package manager's local cache (/usr/local/lib/node/.npm) could be on the hypothetical $JETPACKPATH and a statement like require("sprintf").sprintf("%d", 123) would work (using the NPM's lovely sprintf package).

Several things have to happen to allow this. This is a tracking bug that depends upon the necessary pieces.
(Assignee)

Updated

7 years ago
Assignee: nobody → warner-bugzilla
(Assignee)

Updated

7 years ago
Depends on: 591343, 591338, 552841
(Assignee)

Comment 1

7 years ago
bug 591343: add "module" to module globals, allow setting module.exports to
            replace the "exports" global, try to define "module.id". Do not
            (yet) define "require.main" or "module.filename".
bug 591338: allow require("PACKAGE/MODULE"), incorporate the node.js
            "overlays" feature, find a clean way to let require("panel") be
            the same as require("addon-kit/panel") without making addon-kit
            be too special
bug 552841: encourage jetpack module authors to use version numbers that
            won't confuse CommonJS-compliant implementations

http://wiki.commonjs.org/wiki/Packages/ describes the current CommonJS
thinking on the package.json file (and directory layout).

http://wiki.commonjs.org/wiki/Modules/ describes current thinking on the
module-loader's environment (the globals made available to module code).
(Assignee)

Comment 2

7 years ago
Oops, make that http://wiki.commonjs.org/wiki/Packages and http://wiki.commonjs.org/wiki/Modules
(Assignee)

Updated

7 years ago
Depends on: 614712
(Assignee)

Comment 3

7 years ago
Bug 577782 has another angle on replacing the "exports" global.
Priority: -- → P3
Target Milestone: --- → 1.0
(Assignee)

Comment 4

7 years ago
Looking at a couple of NPM modules installed on my mac, it looks like there
are more things we'll need. My ~/.npmrc is configured with
"root=~/.node_libraries" (which I'll call $ROOT here), and doing "npm install
sprintf" resulted in a $ROOT/sprintf/index.js which appears to be the
intended entry point (when someone does require("sprintf")).

That index.js (which is generated by NPM itself) looks like this:

  #!/usr/bin/env node
  // generated by npm, please don't touch!
  var dep = require('path').join(__dirname, "./../.npm/sprintf/0.1.1/node_modules")
  var depMet = require.paths.indexOf(dep) !== -1
  var bundle = dep.replace(/node_modules$/, 'package/node_modules')
  var bundleMet = require.paths.indexOf(bundle) !== -1
  var from = "./../.npm/sprintf/0.1.1/package/lib/sprintf"
  
  if (!depMet) require.paths.unshift(dep)
  if (!bundleMet) require.paths.unshift(bundle)
  module.exports = require(from)
  
  if (!depMet) {
    var i = require.paths.indexOf(dep)
    if (i !== -1) require.paths.splice(i, 1)
  }
  if (!bundleMet) {
    var i = require.paths.indexOf(bundle)
    if (i !== -1) require.paths.slice(i, 1)
  }
  

which suggests that we'd need a couple of additions, which may or may not fit
well with the rest of Jetpack:

 * __dirname
 * require.paths
 * require("./../PATH/TO/REAL/LIBRARY")

"__dirname" appears to be intended to contain the directory in which the
current file was found, to construct relative paths off of it.

"require.paths" looks to be just like Python's sys.path, an ordered list of
directories on which require() is supposed to search.

From Jetpack's point of view, require(X) should be statically resolvable, so
we can figure out what to copy into the XPI (and record that decision in the
manifest so the runtime knows what to do). To support the import of the
NPM-generated index.js module, the build-time linker would need to analyze
this code, deduce what string is being passed to the "require(from)" call,
then copy that resulting module into the XPI. I think that's too Hard (as in
Halting-Problem hard).

Instead, we might be able to teach the SDK how to look at the unpacked
package directories, which seem to be in
$ROOT/.npm/$PKGNAME/$VERSION/package/ . In each such directory, there is a
package.json file, which has both a "directories.lib" pointer and a "main"
property, and the lib directory contains the actual modules. Also, next to
the $VERSION subdir there is a symlink called "active" which points to the
currently-active version.

A regular $JSPATH control (bug 611495 -style) wouldn't be sufficient for
these, since the directories under $JSPATH should be packages, and the .npm
directory has those extra $VERSION and "package" subdirs. But we could create
a different control, something NPM-specific, like $NPMPATH, or
$JETPACK_NPM_PATH, which would actually search in "./active/package/" for
each package found there. Something like this:

def find(package): # for require("sprintf")
  for p in NPMPATH:
    if package in os.listdir(p):
      packagedir = os.path.join(p, package, "active", "package")
      package_json = json.load(open(os.path.join(packagedir, "package.json")))
      main = os.path.join(packagedir, package_json["main"])
      return load(main)

def find(package, module): # for require("sprintf/submod")
  for p in NPMPATH:
    if package in os.listdir(p):
      packagedir = os.path.join(p, package, "active", "package")
      package_json = json.load(open(os.path.join(packagedir, "package.json")))
      libdir = os.path.join(packagedir, package_json["directories"]["lib"])
      modfile = resolve(libdir, modfile)
      if os.path.exists(modfile):
        return load(modfile)
(Assignee)

Comment 5

7 years ago
Looking more closely at some of the modules installed from NPM, I'm not sure how the "lib" directory should be found. The CommonJS package spec (http://wiki.commonjs.org/wiki/Packages/1.0) says that the "directories" property is optional, and that JS files should be in the "lib" directory, so I assume that the default location should be "lib/" unless .directories.lib overrides that. But many of the NPM modules I see (like underscore-1.1.6) have their source files in the package's root directory, and have no .directories property in their package.json. While others (base32-0.0.3) seem to match the CommonJS spec.

I wonder if we should change the package.json -reading code to use "." (the package root) as the lib directory if there is no ./lib directory and the .directories property does not override it.
(Assignee)

Updated

7 years ago
Depends on: 652227
(Assignee)

Comment 6

7 years ago
bug 652227 added to handle code-in-package-root .
(Assignee)

Comment 7

7 years ago
Created attachment 528242 [details] [diff] [review]
add --npmdir argument

Here's a starting point: it adds an --npmdir argument, which is meant to be pointed at your $NPMROOT/.npm directory. Kinda weird, I know, but it's a lot easier to pull JS libraries from there than from $NPMROOT/$PKGNAME, as described earlier.

Until bug 652227 lands, this will only help use NPM libraries that put their code files in lib/ . There are probably other fixes necessary to use most NPM libraries, so landing this patch is necessary but not sufficient to close this bug.
Attachment #528242 - Flags: feedback?(rFobic)
(In reply to comment #5)
> Looking more closely at some of the modules installed from NPM, I'm not sure
> how the "lib" directory should be found. The CommonJS package spec
> (http://wiki.commonjs.org/wiki/Packages/1.0) says that the "directories"
> property is optional, and that JS files should be in the "lib" directory, so I
> assume that the default location should be "lib/" unless .directories.lib
> overrides that. But many of the NPM modules I see (like underscore-1.1.6) have
> their source files in the package's root directory, and have no .directories
> property in their package.json. While others (base32-0.0.3) seem to match the
> CommonJS spec.

I don't think the "lib" directory has any special meaning anymore in node/npm: http://groups.google.com/group/npm-/browse_thread/thread/840a0f00fcf82759/02091a167cce17e8. I think it's all about the package.json's "main" in terms of what's exported to require("foo").
(Assignee)

Comment 9

7 years ago
oh, wow, that's good to know. And the "directories" property is going away too. And those messages were from last January. Ok, I think that says that bug 652227 is a bad idea, and we should reconsider how we use lib/ . I'll go through that mailing list and see where they've gotten to recently.
Indeed for node `directories.lib` is now `"./"` and there is no way to override that. I guess making our default same will make cross jetpack nodejs packages easier, but then probably we should move our modules to "./" and specify that into `directories.lib`. Also this solves only part of the problem since:

1. Most packages in npm have single entry via `main` property in `package.json` which according to commonjs is a relative path to a module that is returned by `require('packageName')`. We use `main` for completely different thing.

2.Node >=0.4.x looks / npm >= 1.0 installs dependencies into `node_modules` folder (https://github.com/isaacs/npm/tree/master/node_modules) which makes whole thing a lot more complicated for us, to say nothing about poor name `node_modules` which makes no sense in context of jetpack.

3. In node `index.js` files have a special meaning, very similar to `index.html`. For example require("mypackage") will load main module if package.json has `main` or will fall back to "index.js" in the package root. Also require("foo/bar") will load bar/index.js from foo if there is no `bar.js` and `bar` is a folder.

I'm not sure if there is enough amount of packages in npm registry today that would work without 1. 2. or 3. Also I talked with Isaac (NPM Author) and wrote to node mailing list when they were transitioning to this architecture suggest alternative approaches to `node_modules` that would have made jetpack compatibility easier. Unfortunately I got an impression that they don't really care about cross platform packages.
Also I don't quite see diff with recently landed --package-path
Comment on attachment 528242 [details] [diff] [review]
add --npmdir argument

Review of attachment 528242 [details] [diff] [review]:
-----------------------------------------------------------------

This change is no longer relevant, since npm moved to completely new architecture described in: https://bugzilla.mozilla.org/show_bug.cgi?id=614707#c10

Also I think we should close this as wont fix!
Attachment #528242 - Flags: feedback?(rFobic) → feedback-
(Assignee)

Comment 13

6 years ago
sounds ok to me. It'd be nice to be able to use NPM packages, but I think we need some significant restructuring to make it happen, and we probably need to wait for the NPM world to settle down a bit. Maybe in the 2.0 timeframe we can try again.
(Assignee)

Comment 14

6 years ago
Ok, discussion seems to have settled down. Closing this as WONTFIX and we'll re-investigate NPM for a later version.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.