browserify-shim incompatible with other transforms (namely brfs)?

Trying to add brfs to my browserify transforms, ever since the shim moved to package.json config:

"browserify": {
    "transform": [
        "browserify-shim",
        "brfs"
    ]
},

Results in this error every time:

Error: Unable to find a browserify-shim config section in the package.json for C:\My Programs\nodist\bin\node_modules\browserify\lib_empty.js

Results in the same error when attempting to use the transform via the command line using the -t brfs option.

[humidair1999]

Of course I post under the wrong account, like an idiot.

Note that this issue occurs all the way down to 0.x versions of brfs, leading me to believe that browserify-shim is hijacking the transform pipeline in some way. Will try to take a more in-depth look when I get home.

[thlorenz]

The error says you are missing the config section in your package.json, i.e.

{
"browserify-shim": {
  "jquery": "$"
}
}

Adding it as a transform without that section is not sufficient and therefore throws an error.

leading me to believe that browserify-shim is hijacking the transform pipeline in some way

It's not, browserify-shim just activates the first time it is invoked by browserify when it is passed a file that it will transform if a shim is configured.

[humidair1999]

I have fully-configured browser and browserify-shim sections in the package.json already; the shim seems to be complaining about specific files not possessing the config, presumably the files other transforms are manipulating (in brfs's case, node's fs module).

Removing either brfs or browserify-shim from the package.json transforms causes the browserification to work just fine. It's the combination that's not working.

I'll update my repo with brfs later tonight to give an example.

[thlorenz]

Ok, sounds good I'll have a look when it's ready.
Thanks.

[humidair1999]

I've updated my repo with an example:

https://github.com/jkymarsh/grunt-browserify-angular-boilerplate

Just run grunt and you'll see the error I spoke of.

[thlorenz]

Ok, thanks, will have a look tomorrow.

[humidair1999]

So I did take a look at this last night, and I'm working on a fix: as the shim inspects each module, it'll check to see if it's a built-in Nodejs module (by comparing against the running instance of browserify's builtins.js file), and if it is, it'll skip the shim step.

However, I did want to ask: do you think it's really necessary to eject from the script if a module doesn't have a section in the package.json? Maybe just a visual warning would be nice, but literally causing the browserification to fail due to a missing section seems a bit extreme.

[thlorenz]

Thanks, that's kind of what I thought (was swamped today, so no time to look in detail).

literally causing the browserification to fail due to a missing section seems a bit extreme

No it's not too extreme since using browerify-shim transform but not specifying the config is a developer mistake and it's better to fail early and with a splash instead of keeping you guessing why things don't work.

For more info on when to throw and when not philosophy, I'd recommend this post.

Looking forward to a PR that fixes this. If this is reproducible via a test, I'd appreciate if you'd add one as well.

Thanks.

[humidair1999]

No it's not too extreme since using browerify-shim transform but not specifying the config is a developer mistake

With all due respect, this wasn't true in my case - I intentionally left out a config section for the fs module, because one shouldn't be required. This is true for all of the other built-in node modules that browserify has abstractions for, and I could easily see any number of other CommonJS-compatible modules, especially npm modules, that also don't require any configuration.

Even with my fix, it'll only cover the built-in modules. I'm worried that someone will come along with some external npm module that has a client-side-compatible abstraction that supports browserification, and we'll then be shit out of luck.

I'll still work on my proposed fix and see what you think, but I'd urge you to reconsider your stance. I would agree with you if configuration was required, 100% of the time, but it's not. It can comfortably be omitted for at least 2 dozen modules that we know of, and presumably many more that we don't.

[thlorenz]

I intentionally left out a config section for the fs module

The fact that it expected one in the fs module is a bug. I'm not sure why that happened and we should revisit this after you apply the fix, since it may be resolved as well with it.

[humidair1999]

Checked in a fix for this:

humidair1999@7524a10

I had considerable difficulty coming to what I feel is an extensible solution to this problem. It wasn't as straightforward as I expected, since not only are top-level modules shimmed; their dependencies are as well, meaning just inspecting the builtins.js wasn't thorough enough.

Ended up having to inspect the current module being shimmed to see if it resides within the running instance of browserify's directory, and if so, it effectively gets ignored.

See what you think. I'd also like to write some tests, but having trouble getting contextify to build on Windows. Will fix this week.

[thlorenz]

@jkymarsh would you consider PR ing with your fix?

Also I'm not sure if there isn't an easier way to fix this. Couldn't we just ignore the builtin.js file somehow by matching the path to a regex similar to node_modules.browserify.lib.builtin.js$.

I'm thinking of something along the lines of:

if (/node_modules.browserify.lib.builtin.js$/.test(file)) return through();

added around here

LMK what you think.

[humidair1999]

@thlorenz As mentioned in my previous comment, just matching/ignoring the builtins.js isn't thorough enough, because dependencies OF these direct dependencies are also traced.

To be honest, I'm still sticking to my original opinion that config for browserify-shim shouldn't be mandatory for every module used via browserify; it should be completely voluntary, and if you leave config out for a module in the package.json, it shouldn't tank the entire build. It would make everything, including the code/fix for this, significantly simpler.