Allow browser replacement with directory and pass package option through.

[jaredhanson]

This patch allows browser replacements to point to directories, such as:

"browser": {
    "module-a": "./shims/module-a/",
}

When this occurs, rather than returning immediately, the normal resolution procedure kicks in, allowing any package.json files in the directory to be processed. This makes it easy to shim entire modules with entire browser-replacement modules, rather than trying to pick off files internal to the module itself.

This also adds an opts.package option, allowing metadata from the loading package to be passed through to node-resolve.

NOTE: this depends on the pull request here, which adds opts.package to node-resolve itself.

https://github.com/substack/node-resolve/pull/21

This is ultimately in service to the following two issues:

substack/module-deps#7 substack/module-deps#13

I have a remaining patch to module-deps to finalize, which allows package metadata (including any browserify transforms) to survive the resolution process and be applied correctly.

[defunctzombie]

If you want to replace a module you can specify a file or another module name. I do not see what resolving a directory gets you that is currently not available. If you have a separate module it should be installed in node_modules. This change is inconsistent with how modules are resolved in node. On Jun 8, 2013 2:05 PM, "Jared Hanson" notifications@github.com wrote:

This patch allows browser replacements to point to directories, such as:

"browser": { "module-a": "./shims/module-a/", }

When this occurs, rather than returning immediately, the normal resolution procedure kicks in, allowing any package.json files in the directory to be processed. This makes it easy to shim entire modules with entire browser-replacement modules, rather than trying to pick off files internal to the module itself.

This also adds an opts.package option, allowing metadata from the loading package to be passed through to node-resolve.

NOTE: this depends on the pull request here, which adds opts.package to node-resolve itself.

substack/node-resolve#21https://github.com/substack/node-resolve/issues/21

This is ultimately in service to the following two issues:

substack/module-deps#7 https://github.com/substack/module-deps/issues/7 substack/module-deps#13https://github.com/substack/module-deps/issues/13

I have a remaining patch to module-deps to finalize, which allows package metadata (including any browserify transforms) to survive the resolution

process and be applied correctly.

You can merge this Pull Request by running

git pull https://github.com/jaredhanson/node-browser-resolve dir-replace

Or view, comment on, or merge it at:

https://github.com/shtylman/node-browser-resolve/pull/23 Commit Summary

• Allow browser shims to resolve to directories.
• Implement opts.package pass-through to node-resolve.
• Test cases for browser replacement with directory paths.
• Additional test cases for browser replacement with directory paths.
• Make test cases conform to conventions.
• Document package option in README.

File Changes

• M README.mdhttps://github.com/shtylman/node-browser-resolve/pull/23/files#diff-0(1)
• M index.jshttps://github.com/shtylman/node-browser-resolve/pull/23/files#diff-1(71)
• A test/fixtures/node_modules/module-h/foobar-browser/index.jshttps://github.com/shtylman/node-browser-resolve/pull/23/files#diff-2(1)
• A test/fixtures/node_modules/module-h/index.jshttps://github.com/shtylman/node-browser-resolve/pull/23/files#diff-3(1)
• A test/fixtures/node_modules/module-h/package.jsonhttps://github.com/shtylman/node-browser-resolve/pull/23/files#diff-4(6)
• A test/fixtures/node_modules/module-i/foobar-browser/main.jshttps://github.com/shtylman/node-browser-resolve/pull/23/files#diff-5(1)
• A test/fixtures/node_modules/module-i/foobar-browser/package.jsonhttps://github.com/shtylman/node-browser-resolve/pull/23/files#diff-6(3)
• A test/fixtures/node_modules/module-i/index.jshttps://github.com/shtylman/node-browser-resolve/pull/23/files#diff-7(1)
• A test/fixtures/node_modules/module-i/package.jsonhttps://github.com/shtylman/node-browser-resolve/pull/23/files#diff-8(6)
• A test/fixtures/node_modules/module-j/foobar-browser/browser.jshttps://github.com/shtylman/node-browser-resolve/pull/23/files#diff-9(1)
• A test/fixtures/node_modules/module-j/foobar-browser/main.jshttps://github.com/shtylman/node-browser-resolve/pull/23/files#diff-10(1)
• A test/fixtures/node_modules/module-j/foobar-browser/package.jsonhttps://github.com/shtylman/node-browser-resolve/pull/23/files#diff-11(4)
• A test/fixtures/node_modules/module-j/index.jshttps://github.com/shtylman/node-browser-resolve/pull/23/files#diff-12(1)
• A test/fixtures/node_modules/module-j/package.jsonhttps://github.com/shtylman/node-browser-resolve/pull/23/files#diff-13(6)
• A test/fixtures/node_modules/module-k/foobar-browser/browser.jshttps://github.com/shtylman/node-browser-resolve/pull/23/files#diff-14(1)
• A test/fixtures/node_modules/module-k/foobar-browser/main.jshttps://github.com/shtylman/node-browser-resolve/pull/23/files#diff-15(1)
• A test/fixtures/node_modules/module-k/foobar-browser/package.jsonhttps://github.com/shtylman/node-browser-resolve/pull/23/files#diff-16(6)
• A test/fixtures/node_modules/module-k/index.jshttps://github.com/shtylman/node-browser-resolve/pull/23/files#diff-17(1)
• A test/fixtures/node_modules/module-k/package.jsonhttps://github.com/shtylman/node-browser-resolve/pull/23/files#diff-18(6)
• M test/modules.jshttps://github.com/shtylman/node-browser-resolve/pull/23/files#diff-19(90)

Patch Links:

• https://github.com/shtylman/node-browser-resolve/pull/23.patch
• https://github.com/shtylman/node-browser-resolve/pull/23.diff

[jaredhanson]

Can you elaborate on that?

Here's more of an explanation on my part. I have a Node module quux that requires foo. It just so happens that foo is very Node specific, and doesn't make any attempt to provide browser-capable code.

However, I want quux to be browser-capable, so I need a solution to shim out foo itself. As such, I go about implementing foo-browser, and add it to my dependencies (although, all require statements are still require('foo') since node is my primary target).

I want browser support though, so I then add this to my package.json:

"browser": {
    "foo": "./node_modules/foo-browser",
}

Nothing is inconsistent with how node modules are resolved, its exactly the same. This just affords me the opportunity to shim out the entire foo module when building for the browser.

[defunctzombie]

The best way to solve this is for foo to have a browser field. Now any users of foo just get the benefits.

An alternative is to do:

foo: foo-browser

This should work and is a bug if it does not. But I will say that having the field in foo is the better way since it will just work in node and browser without you having to adjust your code.

Apologies for the formatting since I am responding from my phone. On Jun 8, 2013 2:22 PM, "Jared Hanson" notifications@github.com wrote:

Can you elaborate on that?

Here's more of an explanation on my part. I have a Node module quux that requires foo. It just so happens that foo is very Node specific, and doesn't make any attempt to provide browser-capable code.

However, I want quux to be browser-capable, so I need a solution to shim out foo itself. As such, I go about implementing foo-browser, and add it to my dependencies (although, all require statements are still require('foo') since node is my primary target).

I want browser support though, so I then add this to my package.json:

"browser": { "foo": "./node_modules/foo-browser", }

Nothing is inconsistent with how node modules are resolved, its exactly the same. This just affords me the opportunity to shim out the entire foomodule when building for the browser.

— Reply to this email directly or view it on GitHubhttps://github.com/shtylman/node-browser-resolve/pull/23#issuecomment-19156220 .

[jaredhanson]

Can you explain what you mean by the "alternative is to do: foo: foo-browser".

Unless I'm missing something, I can't do the following currently:

"browser": {
    "foo": "./foo-browser",
}

Unless foo-browser is a file. I'm interested in being able to specify a directory as well, so that metadata in package.json of foo-browser can be preserved (for instance, browser transforms in case foo-browser would be written in coffee script, not that I would ever do that). The important bit here is that there may be package-level metadata that's important to preserve when shimming out whole modules, otherwise the shim is lossy.

[defunctzombie]

You can do

module-a: module-b

No ./ required as you are nor replacing with a path. On Jun 8, 2013 2:44 PM, "Jared Hanson" notifications@github.com wrote:

Can you explain what you mean by the "alternative is to do: foo: foo-browser".

Unless I'm missing something, I can't do the following currently:

"browser": { "foo": "./foo-browser", }

Unless foo-browser is a file. I'm interested in being able to specify a directory as well, so that metadata in package.json of foo-browser can be preserved (for instance, browser transforms in case foo-browser would be written in coffee script, not that I would ever do that). The important bit here is that there may be package-level metadata that's important to preserve when shimming out whole modules, otherwise the shim is lossy.

— Reply to this email directly or view it on GitHubhttps://github.com/shtylman/node-browser-resolve/pull/23#issuecomment-19156569 .

[jaredhanson]

I know, but if you do that currently, the package.json of module-b will not be parsed.

[jaredhanson]

Actually, that is a bug I believe. If you module-a: module-b, browserify will fail with an EISDIR error when attempting to read that file. I'll repro this to confirm once I have my branches sorted.

[defunctzombie]

Awesome. I welcome failing examples!

I would also suggest doing the browser field in foo as that is a cleaner solution imho. On Jun 8, 2013 2:53 PM, "Jared Hanson" notifications@github.com wrote:

Actually, that is a bug I believe. If you module-a: module-b, browserify will fail with an EISDIR error when attempting to read that file. I'll repro this to confirm once I have my branches sorted.

— Reply to this email directly or view it on GitHubhttps://github.com/shtylman/node-browser-resolve/pull/23#issuecomment-19156720 .

[jaredhanson]

Agreed on the browser field in foo being potentially preferrable. There are cases where that may not be possible, for example foo's maintainer may refuse to support the browser. In these situations wholesale foo-browser implementations by another party is a reasonable approach.

[jaredhanson]

Confirmed the issue:

"browser": {
    "foo": "bar"
  },

Fails with:

Error: EISDIR, read

[defunctzombie]

Ok, I have updated master to use resolve@0.4.0

For the other changes, it is my understanding that having

{
    "module-a": "module-b"
}

Will get you what you want? Or is this not true? If it is true, I would like to rework the changeset to do that.

[jaredhanson]

Yes, that's the goal. I'd reiterate that module-b may have a package.json file which needs to be processed to resolve its main file. It's my opinion that that should go through node-resolve's normal processing so that its fully conformant with the way node handles package.json (or defaulting to /index).

I'm happy to rework the patch, but I'm not sure what other approach would be correct. Let me know what your alternative would be.

[jaredhanson]

FYI, for an example of how I'm using this, see here: https://github.com/anchorjs/url/blob/master/package.json

There's a couple other issues that need to be addressed, but I'll make some proposals after this one is worked out, since this is the most important.

[defunctzombie]

Based on that package.json, you shouldn't need any browser field. The deps are being used as is...unless you intended for something different to happen?

On Sun, Jun 9, 2013 at 9:02 AM, Jared Hanson notifications@github.comwrote:

FYI, for an example of how I'm using this, see here: https://github.com/anchorjs/url/blob/master/package.json

There's a couple other issues that need to be addressed, but I'll make some proposals after this one is worked out, since this is the most important.

— Reply to this email directly or view it on GitHubhttps://github.com/shtylman/node-browser-resolve/pull/23#issuecomment-19168491 .

[jaredhanson]

OK, that's a bad example. Ignore it. One of the important bits there is that I'm using my own implementations of querystring, not the browserify version. (punycode is also "core", but browser-builtins doesn't supply it, so that's listed as well). Normally you wouldn't see "querystring" listed in dependencies.

This is one of those other proposals I mentioned, namely that I think the "browser" field should be able to be used for core module implementations. If I take the browser field out of that example, it'll fall back to browserify's builtins.

[jaredhanson]

Regardless of that, the example illustrates shimming out entire modules (in this case "core"-querystring with my own querystring).

The same applies equally well to module-a: module-b. (I don't have any public repo with that case, but I've tested that as well).

[defunctzombie]

Oh, I see. Ok. I will fix the module-a: module-b issue which should resolve the querystring and punycode issues and allow you to use the proper browser field syntax.

On Sun, Jun 9, 2013 at 9:09 AM, Jared Hanson notifications@github.comwrote:

Regardless of that, the example illustrates shimming out entire modules (in this case "core"-querystring with my own querystring).

The same applies equally well to module-a: module-b. (I don't have any public repo with that case, but I've tested that as well).

— Reply to this email directly or view it on GitHubhttps://github.com/shtylman/node-browser-resolve/pull/23#issuecomment-19168632 .

[jaredhanson]

Great! Also note the opts.package addition, which is important for browserify so that it knows metadata for all modules in a package. It's important to preserve that, as evidenced in this example by the fact that the dependencies also have:

"browserify": {
    "transform": "deamdify"
  },

Because they are written in AMD-style, and need that transform prior to going through the browserify pipeline.

[jaredhanson]

Let me know what your approach to that is, without processing it as a directory complete with package.json parsing.

[defunctzombie]

I have pushed changes to master which allow for "module": "alt-module" to work. Also added tests for this. If you could try it out and let me know if it solves that problem we can start looking at what the other issues are.

On Sun, Jun 9, 2013 at 9:32 AM, Jared Hanson notifications@github.comwrote:

Let me know what your approach to that is, without processing it as a directory complete with package.json parsing.

— Reply to this email directly or view it on GitHubhttps://github.com/shtylman/node-browser-resolve/pull/23#issuecomment-19169012 .

[jaredhanson]

I'll take a look tonight. It will be important however to pass through the opts.package and callback pkg argument that node-resolve now support.