NPM support: Add support for resolving main from npm's package.json

[guilleiguaran]

Similarly to Bower, this allow to require NPM libraries using only the name of package finding automatically the main entry point in the package.json. E.g: In the application.js file users would be able to do:

//= require jquery

instead of

//= require jquery/dist/jquery

[rafaelfranca]

cc @bouk since you worked in the npm directive.

[schneems]

I think this will also end up catching this case #169, maybe we could resolve that here as well?

[guilleiguaran]

I'm not sure if npm/bower modules are the right place to fix #169 since in those modules we are just extending the behavior defined in Resolve module.

I would check if we can fix it in Resolve module since the bug seems to be happening with assets provided via gems.

[bouk]

I like this, I think it makes a lot of sense to include it in Sprockets. Made a couple of comments that are about sticking to the nodejs specification of how the package resolving works, which I think is important to stick to so we don't end up with different behavior

[bouk]

Looking at https://nodejs.org/api/modules.html#modules_all_together, I don't think array as the main field is actually acceptable

[guilleiguaran]

fixed 👍

[bouk]

shouldn't this be joined to the directory? I think the reason your test succeeds for this is that it's using the default index behavior already built into sprockets

[guilleiguaran]

fixed 👍

[bouk]

I can't find any reference to this in https://nodejs.org/api/modules.html#modules_all_together, is this actually true?

[guilleiguaran]

Honestly I'm not sure, but is my interpretation of LOAD_AS_DIRECTORY(X) that package.json is immediately inside of folder X not in subfolders:

LOAD_AS_DIRECTORY(X)
1. If X/package.json is a file,
   a. Parse X/package.json, and look for "main" field.
   b. let M = X + (json main field)
   c. LOAD_AS_FILE(M)

How can we be 100% sure of this?

[guilleiguaran]

From common.js specs: http://wiki.commonjs.org/wiki/Packages/1.0

Each package must provide a top-level package descriptor, "package.json".

(thanks @buritica for reference)

[bouk]

LOAD_AS_DIRECTORY can be any directory though, X is whatever is being loaded. So if the directory being referenced from the require call has a package.json in it, then that file should be interpreted. I tried it out locally:

$ ls
total 0
drwxr-xr-x 3 bouke 102 Oct 19 10:55 test/
$ ls test/a/b
total 8.0K
-rw-r--r-- 1 bouke 20 Oct 19 10:58 main.js
-rw-r--r-- 1 bouke 22 Oct 19 10:58 package.json
$ cat test/a/b/package.json
{"main": "./main.js"}
$ cat test/a/b/main.js
module.exports = 1;
$ node -e "console.log(require('./test/a/b'))"
1

As you can see, the algorithm will always look for a package.json, no matter whether it's a package directly inside the path being resolved or not

[guilleiguaran]

Thanks for clarifying, I've fixed the code and added a test-case to cover this

[bouk]

I think you can drop the .freeze everywhere, because you already have frozen_string_literal: true

[guilleiguaran]

👍