UMD wrapper doesn't work with webpack

[josephfrazier]

On the current master branch (96cfb9c), the following test illustrates the issue:

npm install --global webpack
npm run build
echo "console.log(require('./'))" > t.js
webpack t.js w.js
node w.js

This patch partially reverts 31d778c in order to support webpack.

On my machine, here's the test case output before this patch:

/Users/josephfrazier/workspace/webextension-polyfill/w.js:75
    module.exports = factory(!(function webpackMissingModule() { var e = new Error("Cannot find module \".\""); e.code = 'MODULE_NOT_FOUND'; throw e; }()), exports, module);
																	     ^

Error: Cannot find module "."
at webpackMissingModule (/Users/josephfrazier/workspace/webextension-polyfill/w.js:75:78)
at apiMetadata.alarms.clear.minArgs (/Users/josephfrazier/workspace/webextension-polyfill/w.js:75:156)
at Object.<anonymous> (/Users/josephfrazier/workspace/webextension-polyfill/w.js:89:2)
at Object.module.exports.module.deprecate (/Users/josephfrazier/workspace/webextension-polyfill/w.js:956:30)
at __webpack_require__ (/Users/josephfrazier/workspace/webextension-polyfill/w.js:20:30)
at Object.<anonymous> (/Users/josephfrazier/workspace/webextension-polyfill/w.js:1002:13)
at __webpack_require__ (/Users/josephfrazier/workspace/webextension-polyfill/w.js:20:30)
at /Users/josephfrazier/workspace/webextension-polyfill/w.js:66:18
at Object.<anonymous> (/Users/josephfrazier/workspace/webextension-polyfill/w.js:69:10)
at Module._compile (module.js:571:32)

And here's the output after:

/Users/josephfrazier/workspace/webextension-polyfill/w.js:415
return wrapObject(chrome, staticWrappers, apiMetadata);
		  ^

ReferenceError: chrome is not defined
at wrapAPIs (/Users/josephfrazier/workspace/webextension-polyfill/w.js:415:23)
at Object.module.exports.alarms.clear.minArgs (/Users/josephfrazier/workspace/webextension-polyfill/w.js:420:20)
at __webpack_require__ (/Users/josephfrazier/workspace/webextension-polyfill/w.js:20:30)
at Object.<anonymous> (/Users/josephfrazier/workspace/webextension-polyfill/w.js:947:13)
at __webpack_require__ (/Users/josephfrazier/workspace/webextension-polyfill/w.js:20:30)
at /Users/josephfrazier/workspace/webextension-polyfill/w.js:66:18
at Object.<anonymous> (/Users/josephfrazier/workspace/webextension-polyfill/w.js:69:10)
at Module._compile (module.js:571:32)
at Object.Module._extensions..js (module.js:580:10)
at Module.load (module.js:488:32)

Note that this second error is expected, since Node doesn't provide the chrome global. The code in w.js can be pasted into a browser, where it works correctly.

Related issues:

• https://github.com/mozilla/webextension-polyfill/pull/17/files/af4da3151068a06d30a16d0ec8049f2b02990ae0#diff-a8dc0200482fd536ca439ed8bcf763c2
• Publish to npm for use with browserify/webpack/etc. #32

[rpl]

@josephfrazier Thanks for tracking this down, I took a brief look at it and I think that we can find a better solution than reverting that part.

In particular, browserify doesn't seem to have any issue with the build UMD module, but webpack does an analysis of the bundled modules and it is confused by the UMD wrapper (also, it seems to be a known issue: e.g. webpack/webpack#319).

As an experiment, I removed any 'require' from the built UMD module (we don't require anything, and so the require in the UMD wrapper is not really used by anything inside the module) and after this change webpack is able to load the UMD wrapped module without any issue.

I think that we can fix this issue with one of the following two strategies:

• use a custom UMD wrapper that doesn't pass any "require" var as the factory parameter

• or, use babel to build the UMD wrapped file from the original sources (e.g. similarly to how this other project has solved a similar issue with webpack and their UMD wrapped module: https://github.com/websdk/nap/pull/27/files)

How that sounds to you?

[josephfrazier]

Thanks for the quick response. I know we've discussed this before, but I still don't understand why simply using the CommonJS syntax in src/ files is a problem. As I recall, you brought up two points:

• But I'm a bit concerned that by looking at the sources (without looking at the building process) this require can be misinterpreted, e.g. as "we can require other external modules in the browser-polyfill sources" which is not true, we are more probably going to keep this as module as simple as possible, without any external dependencies, to keep is simpler to use as a "simple script tag included in the page" or as an AMD dependency.

This is addressed by the following source code comment: "Note that require does NOT work in general"

• But the require will not work correctly if the source version is loaded accidentally instead of the build version as a plain tag script added to an extension page, or as an AMD module.

Yes, but that's also the case currently, due to the check that was added here, so we'd just be throwing a different type of Error. I went into a bit more detail on this here.

---

Note also that the change I'm proposing here doesn't affect the code output to dist/ (which is still included in the package, for anyone who wants a prebuilt version). It's also more conventional in the npm ecosystem to publish packages with a CommonJS entrypoint, since it's assumed that they're either being used directly in Node, or compiled with browserify/webpack/etc.

If you're able to find an alternative solution like the two you mentioned, that works too. I'm just trying to see the downsides of my proposal.

EDIT: It is a bit unfortunate that we have to work around webpack specifically, since browserify works (as you mentioned). However, making this change also has the side effect of reducing bundle size, since the UMD boilerplate isn't unnecessarily included into webpack/browserify builds.

[rpl]

Hi @josephfrazier, I understand your point, but once we agreed on the current approach and we've been finally able to merge the UMD module, I would prefer to fix the issue with webpack as soon as possible, and a solution which is consistent with the approach which we already agreed on is probably the fastest option.

In #35 I tried the approach that I described in the previous comment and it seems to work great for both browserify and webpack and it also a pretty small change (fully accomplished by only changing the umd build step).

Also, the above PR contains a different testing strategy which should fully ensure that the "module bundlers" that we are interested in are going to work correctly and don't regress.

Let me know how #35 looks to you and that it also works for you as expected.

[josephfrazier]

Hi @rpl, thanks for the quick turnaround with #35. I just tried it and confirmed that the dist/ files work with the webpack build in my project. It's also nice to see the extra bundler tests, given that the entrypoint isn't CommonJS just a simple "module.exports = ..."-type file and therefore isn't essentially guaranteed to keep working with the bundlers.

I wish I had tested webpack back in #17, then I would have pushed harder for the CommonJS approach, but #35 looks like it solves the problem too (with the small caveat that you still can't npm install from a github url, which is useful for trying out changes that haven't yet been published to npm).

Feel free to close this once #35 is merged. Thanks again!

[rpl]

@josephfrazier Thanks for the confirmation, I've merged #35 and released it on npm as 0.1.1 (and also added some automation for the npm publishing process, which will make the next releases easier).

Also, thanks again for contributing the initial changes for the UMD module wrapping and being able to finally release it on npm, you have been absolutely awesome.

PS: I agree that it would be nice if we can provide a simpler way to be able to test pre-releases versions, and we can definitely rethink it in the future (e.g. when we put together a "workflow", and some automation, related to the generation of updates on the api-metadata.json, these metadata are going to need some updates from time to time, to keep them synchronized with the updates on the API schema in mozilla-central).

[josephfrazier]

@rpl Thanks for quickly merging #35 and releasing! I should be able to use webextension-polyfill in my projects now. It's great to hear that future releases have been automated too, as that should make it much easier to distribute updates. Thanks again for all the work you and the rest of the team have done on this, it's a big help with cross-browser extension development.

Regarding pre-release testing, if you were to merge this PR, it should be as simple as npm install https://github.com/mozilla/webextension-polyfill#COMMIT_HASH, for projects that use a bundler.