Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Webpack production mode compatibility #6956

Merged
merged 2 commits into from
Jul 13, 2018
Merged

Conversation

mourner
Copy link
Member

@mourner mourner commented Jul 12, 2018

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
Copy link
Member Author

mourner commented Jul 12, 2018

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
Copy link
Contributor

bhousel commented Jul 12, 2018

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
Copy link
Contributor

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
Copy link
Member Author

mourner commented Jul 12, 2018

@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.

Copy link
Contributor

@jfirebaugh jfirebaugh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -29,6 +29,7 @@ export const plugins = () => [
include: 'src/shaders/index.js'
}),
commonjs({
ignoreGlobal: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mourner mourner merged commit f55e548 into master Jul 13, 2018
@mourner mourner deleted the fix-webpack-global branch July 13, 2018 12:36
@george3447
Copy link

@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
Copy link
Contributor

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants