Fix Webpack production mode compatibility

[mourner]

A proposal for fixing #4359. Sets ignoreGlobal: true option for rollup-plugin-commonjs plugin, turning off its handling of the global keyword in CommonJS modules, and upgrades the plugin to the latest version (because the option didn't work properly in the previous one). In particular, it stops transforming top-level this reference into a variable that contains the typeof global check (which consequently triggered Webpack issues).

One consequence of the fix is that GL JS will throw a runtime error when it encounters a reference to the global keyword. However, there are no such instances in the bundle currently — the only real reference I found (in the browserified node assert module) gets treeshaken away because it's in unused methods. So in theory this fix won't bring much trouble unless we plan on using Node-targeted modules with the global keyword in future.

Thanks a lot to @Aendrew who provided a minimal repro that helped me find this fix.

[mourner]

I must also mention that an alternative way to achieve a working Webpack build would be to release new versions of whoots-js and shelf-pack (cc @bhousel), adding a module field to package.json. Currently they're treated as CommonJS by Rollup despite being authored as ES modules because instead of module field, they use jsnext:main which is considered deprecated and is no longer handled by modern bundlers. We should definitely do this, but I'd still merge this PR because we might want to use some CommonJS modules in future, even after eliminating the current uses.

[bhousel]

I must also mention that an alternative way to achieve a working Webpack build would be to release new versions of whoots-js and shelf-pack (cc @bhousel), adding a module field to package.json.

Thanks for flagging @mourner, I'll take care of this now 👍

[anandthakker]

I must also mention that an alternative way to achieve a working Webpack build would be to release new versions of whoots-js and shelf-pack (cc @bhousel), adding a module field to package.json.

@mourner can you explain the relationship between ignoreGlobal: true and rollup-plugin-commonjs's handling of these cjs dependencies that expose ES modules?

[mourner]

@anandthakker sure. If you look into dist/mapbox-gl-dev.js on master, you can see the following line at the top introduced by rollup-plugin-commonjs:

var commonjsGlobal = typeof window !== 'undefined' ? window : typeof global !== 'undefined' ? global : typeof self !== 'undefined' ? self : {};

When Rollup encounters a commonjs-style UMD build such as previous versions of whoots-js and shelf-pack (now updated thanks to @bhousel), it sees a top-level this keyword (used by the UMD wrapper) and replaces it with a commonjsGlobal reference:

var index_umd = createCommonjsModule(function (module, exports) {
(function (global, factory) {
	'object' === 'object' && 'object' !== 'undefined' ? module.exports = factory() :
	typeof undefined === 'function' && undefined.amd ? undefined(factory) :
	(global.ShelfPack = factory());
}(commonjsGlobal, (function () {

Setting ignoreGlobal: true disables that top-level this substitution, so the bundle no longer needs the commonjsGlobal definition and it gets treeshaken away in the production build.

[jfirebaugh]

Good find! As I recall, the typeof global check was buried much deeper in a transitive dependency with browserify, so it seems that switching to rollup had yet another benefit.

[jfirebaugh]

Add a comment here outlining why we're using this option and linking to this PR.

[george3447]

@jfirebaugh, @mourner

Could you please tell, is this PR included in latest npm release (0.47.0-beta.1), if not is there any tentative date planned for next release including this PR. It would be really helpful if you can provide some info regarding the same. Thanks.

[jfirebaugh]

It's not included in 0.47.0-beta.1, but if some of the folks here who have been affected by the issue can try it out (using a build from master) and confirm that it resolves the issue for them and doesn't cause any new issues, we'll include it in 0.47.0 final (due out Wednesday 7/18).

[aendra-rininsland]

@jfirebaugh @mourner Just tried it with my current project, LGTM 👍