fix browserify + webpack

[mollymerp]

Potential fix for the ongoing webpack saga 😭
@anandthakker @lucaswoj

Launch Checklist

• briefly describe the changes in this PR
• write tests for all new functionality
• document any changes to public APIs
• post benchmark scores
• manually test the debug page

[mollymerp]

manually tested that this works with webpack using https://gist.github.com/mollymerp/369e1287a473966e29fe92569495687e and editing the main field of node_modules/mapbox-gl/package.json to be src/index.js

[anandthakker]

@mollymerp oooof thanks for figuring this out 😕 🏅 🌟

I wonder if we should add browserify and webpack build tests. Browserify would be easy to do without adding a new dependency. Webpack's a heavy dep to add just for a unit test... maybe we could do that one only on CI?

[anandthakker]

cc @mourner @jfirebaugh as well -- note that this reverts the change @mourner proposed in #3551

[lucaswoj]

Nothing here that we did have a webpack test at one point: f6bcd06#diff-cf4d6d7603881cba61aea62bcbf7e77d

[lucaswoj]

Did you uncover the root cause of the build failures? I would much prefer a solution that doesn't require special steps for either webpack of browserify.

[anandthakker]

@lucaswoj I'm not 100% on this, but I think the problem is that a browserified bundle is not, in fact, a valid CommonJS module. In particular, my hypothesis is:

• the browserified bundle will itself have calls to require, but they are meant to refer to a private require function supplied by browserify in the bundle prelude.
• when you try to _re_browserify that bundle (or rather, when you try to browserify a source file that requires it), those private require calls get treated as if they were "real" CommonJS requires

[mollymerp]

I believe the root cause is that because the bundle we create in the /dist folder is standalone UMD module (what browserify creates), and so it wont work out of the box for webpack as @anandthakker said. I think in order for this to work in webpack there will be special instructions that need to go in the config file. Either what i've proposed above, or this https://webpack.js.org/concepts/output/#output-librarytarget set to umd

[anandthakker]

a browserified bundle is not, in fact, a valid CommonJS module

^ this might not be accurate, actually -- I think as Molly said it's a UMD module, which should work in a CommonJS environment (Node); but it doesn't work when re-browserified because of the require issue

[anandthakker]

Just found https://github.com/calvinmetcalf/derequire -- it's possible that including this transform when building dist/mapbox-gl.js might allow browserify to work, in which case we could leave main set to dist/mapbox-gl.js.

[jfirebaugh]

To me this looks like a bug in browserify's static analysis. A very simple example that demonstrates what's going on is:

module.exports = function(require) {
    require('nonesuch');
};

If you attempt to browserify this, you get:

Error: Cannot find module 'nonesuch' from '/Users/john/Development/mapbox-gl-js'
    at /Users/john/Development/mapbox-gl-js/node_modules/browser-resolve/node_modules/resolve/lib/async.js:46:17
    at process (/Users/john/Development/mapbox-gl-js/node_modules/browser-resolve/node_modules/resolve/lib/async.js:173:43)
    at ondir (/Users/john/Development/mapbox-gl-js/node_modules/browser-resolve/node_modules/resolve/lib/async.js:188:17)
    at load (/Users/john/Development/mapbox-gl-js/node_modules/browser-resolve/node_modules/resolve/lib/async.js:69:43)
    at onex (/Users/john/Development/mapbox-gl-js/node_modules/browser-resolve/node_modules/resolve/lib/async.js:92:31)
    at /Users/john/Development/mapbox-gl-js/node_modules/browser-resolve/node_modules/resolve/lib/async.js:22:47
    at FSReqWrap.oncomplete (fs.js:82:15)

Browserify shouldn't assume that any local variable that happens to be named require refers to the global.

[jfirebaugh]

Relevant upstream tickets:

• Browserify a Browserified module browserify/browserify#374
• Make browserify recognize its own bundles browserify/browserify#1151
• if require is aliased, detective incorrectly returns uses of it as dependencies browserify/detective#22

[anandthakker]

@jfirebaugh yah, I agree, but I don't think we should count on it getting fixed anytime soon, alas

…

On Wed, Mar 22, 2017 at 7:18 PM John Firebaugh ***@***.***> wrote: To me this looks like a bug in browserify's static analysis. A very simple example that demonstrates what's going on is: module.exports = function(require) { require('nonesuch'); }; If you attempt to browserify this, you get: Error: Cannot find module 'nonesuch' from '/Users/john/Development/mapbox-gl-js' at /Users/john/Development/mapbox-gl-js/node_modules/browser-resolve/node_modules/resolve/lib/async.js:46:17 at process (/Users/john/Development/mapbox-gl-js/node_modules/browser-resolve/node_modules/resolve/lib/async.js:173:43) at ondir (/Users/john/Development/mapbox-gl-js/node_modules/browser-resolve/node_modules/resolve/lib/async.js:188:17) at load (/Users/john/Development/mapbox-gl-js/node_modules/browser-resolve/node_modules/resolve/lib/async.js:69:43) at onex (/Users/john/Development/mapbox-gl-js/node_modules/browser-resolve/node_modules/resolve/lib/async.js:92:31) at /Users/john/Development/mapbox-gl-js/node_modules/browser-resolve/node_modules/resolve/lib/async.js:22:47 at FSReqWrap.oncomplete (fs.js:82:15) Browserify shouldn't assume that any local variable that happens to be named require refers to the global. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4462 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEvmR1HxzJla4LRu7GPMMNbfa2LUJuNYks5roaw8gaJpZM4MjMos> .

[jfirebaugh]

Agreed. In the meantime, derequire (or perhaps derequire-transform) looks like the most commonly used workaround.

[anandthakker]

I'm enjoying this comment in the README of derequire-transform

It will only help in the case of a library that does something unwise like defining a local require function (cough ember cough)

since that's exactly what browserify does.

[jfirebaugh]

The derequire approach works! #4486