Add CommonJS export to package.json

[josephfrazier]

This addresses issue 7 by making it possible for users to run:

npm install https://github.com/mozilla/webextension-polyfill

and download a module that they can use with a bundler as follows:

import browser from 'webextension-polyfill';

Also, add a prepublish script so that users don't need to run grunt
manually. In addition, specify files in package.json so that this
module can be published to npm without including miscellanea. This can
be verified by running:

npm pack && tar -tvf webextension-polyfill-0.1.0.tgz

The dist/ files would be generated upon publishing the package,
meaning that users could run npm install webextension-polyfill, then
load the following files into their extension, without having to build
them:

node_modules/webextension-polyfill/dist/browser-polyfill.js
node_modules/webextension-polyfill/dist/browser-polyfill.min.js

[kumar303]

Awesome, looks good to me but someone else should review it too. Maybe @rpl ?

[rpl]

Hi @josephfrazier! First of all thanks for looking into this!

Provide the webextension-polyfill as an npm package is definitely something that we should do, what concerns me a bit about how this PR achieves this goal is that the browserify loader is going to be always included in the final built file, even when the addon that includes this library is not using browserify (or any other module loader).

How about trying to wrap it using the UMD format instead?

By wrapping it in an UMD format (Universal Module Format), no browserify loader will be included in final javascript file, nevertheless it would be loadable using a simple tag script, an AMD module loader or a CommonJS module loader (e.g. browserify, webpack etc.)

As an example, it seems that the grunt-umd would be able to wrap our library as described above,
the changes needed to integrate are not not far from the ones that you are already using with browserify in this pull request, and the final javascript will be generated from the pipeline that follows:

• injecting the json file into the initial source file of the polyfill (dist/browser-polyfill.js)
• wrapping the sources into an UMD module wrapper (which overwrites dist/browser-polyfill.js)
• generate a minified version of the final UMD module (which generates dist/browser-polyfill.min.js)

I've briefly tried grunt-umd locally and it seems that the following changes are enough to integrate it:

• load and configure grunt-umd in the Gruntfile

diff --git a/Gruntfile.js b/Gruntfile.js
index 7f6e503..6d2929b 100644
--- a/Gruntfile.js
+++ b/Gruntfile.js
@@ -55,6 +55,15 @@ module.exports = function(grunt) {
       },
     },

+    umd: {
+      all: {
+        src: "dist/browser-polyfill.js",
+        objectToExport: "browserPolyfill",
+        globalAlias: "browser",
+        amdModuleId: "webextension-polyfill",
+      },
+    },
+
     "closure-compiler": {
       dist: {
         files: {
@@ -88,8 +97,9 @@ module.exports = function(grunt) {

   grunt.loadNpmTasks("gruntify-eslint");
   grunt.loadNpmTasks("grunt-replace");
+  grunt.loadNpmTasks("grunt-umd");
   grunt.loadNpmTasks("grunt-coveralls");
   require("google-closure-compiler").grunt(grunt);

-  grunt.registerTask("default", ["eslint", "replace", "closure-compiler"]);
+  grunt.registerTask("default", ["eslint", "replace", "umd", "closure-compiler"]);
 }; 

• small tweaks on the javascript sources (similar to the one that you are currently applying in this PR)

diff --git a/src/browser-polyfill.js b/src/browser-polyfill.js
index b7041e7..2cbb02e 100644
--- a/src/browser-polyfill.js
+++ b/src/browser-polyfill.js
@@ -6,6 +6,8 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 "use strict";

+var browserPolyfill; // eslint-disable-line no-unused-vars
+
 if (typeof browser === "undefined") {
   // Wrapping the bulk of this polyfill in a one-time-use function is a minor
   // optimization for Firefox. Since Spidermonkey does not fully parse the
@@ -336,5 +338,7 @@ if (typeof browser === "undefined") {
     return wrapObject(chrome, staticWrappers, apiMetadata);
   };

-  this.browser = wrapAPIs();
+  browserPolyfill = wrapAPIs();
+} else {
+  browserPolyfill = browser;
 }

• small changes to the package.json (which are almost the same ones that you are applying in this PR)

Let me know what you think about this alternative strategy.

[josephfrazier]

Thanks for the comprehensive feedback, @rpl! I definitely understand the concern about having browserify boilerplate in the built file, so I'll push some changes that use grunt-umd instead.

[rpl]

Hi @josephfrazier
Thank you so much for these changes, I added a bunch of additional comments/ideas.

Besides the comments that follows, there is one additional missing piece that I would like to have once we are ready to land it:

• additional test cases to test that the built file is working correctly as a CommonJS module bundled using webpack/browserify and as an AMD module (we don't need to re-run everyone of the existent test cases in this new scenarios, but we need to test that it is going to return the expected polyfill object in the two new "usage modes")

The setupTestDOMWindow helper defined in test/setup.js doesn't currently support these scenarios "out of the box", but it shouldn't be too hard to adapt it to support these additional scenarios.

(I'll take a deeper look into it asap and I'm going provide more ideas about the strategy/needed changes in the test suite)

[rpl]

The build step is still needed, and so I would live the grunt command here (while we should add a section that explains how to include it as a npm dependencies once we have done with this, I would also be ok to defer it as a follow up and handle the additional documentation in a separate pull request)

[josephfrazier]

Since npm run build was added to package.json as the prepublish script, it will run as part of the npm install lifecycle. Granted, this behavior is deprecated and will be removed in npm v5 (see here for details), so this part of the documentation will need to be changed anyway.

What if I changed the line to this?

npm run build # if you're using npm v5+

---

EDIT: I've been thinking about the need to document usage as npm install webextension-polyfill, but I hadn't gotten around to writing anything yet. I think it would be nice if we deferred that until the package is actually published, to make sure that the command works as described.

[rpl]

how about defining both prepublish and prepare? If I read the deprecation note correctly, it would make the package able to work correctly across the npm versions, am I right?

[rpl]

@josephfrazier we should also take into account that the devDependencies are not retrieved when the package is installed as a dependency of another one, and so I'm not entirely sure that defining prepublish/prepare will be enough to get the the dist files correctly built in this scenario.

[josephfrazier]

how about defining both prepublish and prepare? If I read the deprecation note correctly, it would make the package able to work correctly across the npm versions, am I right?

I just tried defining both prepublish and prepare, and it causes the build to happen twice when npm install is run using npm v4. So, it would work correctly across npm versions, but just takes a little extra time on npm v4, whereas npm v3 runs the build only once (as will npm v5 once it's released).

We can fix this by using in-publish to prevent the prepublish script from running upon npm install with npm v4. Alternatively, if all the developers who will be publishing this are already using npm v4, we can change prepublish to prepublishOnly, as detailed in the deprecation note linked in my previous comment.

we should also take into account that the devDependencies are not retrieved when the package is installed as a dependency of another one, and so I'm not entirely sure that defining prepublish/prepare will be enough to get the the dist files correctly built in this scenario

Note that none of these scripts (prepublish, prepare, prepublishOnly) would be invoked when a user runs npm install webextension-polyfill (which is different from running npm install inside a clone of this repo). Since running npm publish invokes the prepublish script, the dist/ files will get built and included in the published package, so a user who runs npm install webextension-polyfill will have them without needing to install the devDependencies.

As I mentioned when I opened the PR, you can verify this by running the following:

npm pack && tar -tvf webextension-polyfill-0.1.0.tgz

[rpl]

My guess is that you are preferring the require here so that the github repo can be used as an npm dependencies with browserify/webpack without the need for a build step.

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.

I would vote to keep it as {/* include("...") */} like it was.

@kmaglione what do you think about this? do you agree with my point above?

[josephfrazier]

My guess is that you are preferring the require here so that the github repo can be used as an npm dependencies with browserify/webpack without the need for a build step.

That's a side benefit, yes, but my goal this time was actually to make it so that this package can be imported or require()ed by other projects without including UMD boilerplate, while the build process still produces a UMD module for use in a script tag.

But I'm a bit concerned that by looking at the sources (without looking at the building process) this require can be misinterpreted

I definitely agree here, and even felt a little dirty writing the grunt-replace rule for the require() syntax (especially since it has to strip out the leading ../ in the path) 😝

However, now that I think about it, there is a nice incidental effect of having it this way: It makes it much harder for people to accidentally use the source version of the polyfill (which happened in #5)

Ultimately, though, I'm not really devoted to this syntax, and would happily switch back to {/* include("...") */} if that's what the team would prefer.

[rpl]

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.

We can prevent the "CommonJS" loader (and bundler) to pick the wrong file by changing the "main" property in the package.json to the dist file.

Let's wait a feedback from @kmaglione on this (but I vote for switching back to the {/* include("...") */} ;-)

[josephfrazier]

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, that's the point :) Instead of silently failing because the following line is valid JS, there would be an Uncaught ReferenceError. Well, if both module and require are defined globally, there could still be a silent failure, but this seems unlikely.

    const apiMetadata = {/* include("api-metadata.json") */};

We can prevent the "CommonJS" loader (and bundler) to pick the wrong file by changing the "main" property in the package.json to the dist file.

Maybe we're misunderstanding each other here, but bundlers like browserify and webpack automatically make sure that require/module/etc are defined correctly, so they should use src/browser-polyfill.js. Of course, if a user wanted to npm install webextension-polyfill, then use a script tag to include the polyfill, they could be slightly misled by seeing "main": "src/browser-polyfill.js" in package.json, but I think that can be easily handled in documentation.

Let's wait a feedback from @kmaglione on this (but I vote for switching back to the {/* include("...") */} ;-)

Agreed on waiting for feedback :) Another way to go about this would be to switch back to {/* include("...") */}, but then change it to something that throws an error if it's not replaced. That would also make it more obvious to the user if they accidentally use the src file in a script tag.

[rpl]

I mostly like that by using the unit grunt-umd template we didn't need to rename the global alias to be able to export it, but I'm a bit worried that we have changed the eslint config to enable the "commonjs" mode, which can detect as "valid" something that is valid in a regular commonjs module but that we probably want to be mentioned as errors (e.g. usage of the any usage of the require or other globals available in a nodejs module).

How about reverting the change to .eslintrc and opt to use // eslint-disable-line no-undef on these two lines?

something like:

diff --git a/src/browser-polyfill.js b/src/browser-polyfill.js
index 5d55403..f1ab55c 100644
--- a/src/browser-polyfill.js
+++ b/src/browser-polyfill.js
@@ -336,7 +336,7 @@ if (typeof browser === "undefined") {
     return wrapObject(chrome, staticWrappers, apiMetadata);
   };

-  module.exports = wrapAPIs();
+  module.exports = wrapAPIs(); // eslint-disable-line no-undef
 } else {
-  module.exports = browser;
+  module.exports = browser; // eslint-disable-line no-undef
 }

If we opt for this solution (grunt-umd 'unit' template and module.exports) instead of renaming the global as I was suggesting in my previous comment, I would also add an inline comment near to this code, to explain that these module.exports are defined in the UMD wrapper.

[josephfrazier]

Ooh, that's a good point about "commonjs" mode causing eslint to be too lenient. I've pushed up 35e8e7a with the suggested changes. Of course, if we go back to renaming the global, this will have to be reverted/etc.

[josephfrazier]

Cheers for the comments, @rpl. I've responded and I look forward to seeing where the discussion goes.

As for the bundler/AMD testing: I took a very quick shot at testing browserify/webpack in Node (without integrating it into the main test suite). I don't know if this is a good way to go about it, but you can at least have a look at it here: josephfrazier@ca21b4f

[rpl]

@josephfrazier Thanks for the updates, for the detailed comments and also for starting to look into the test case!

I'm not sure that it is really necessary to test the "commonjs bundled module"-mode with both browserify and webpack (there should be no difference between the two tools from this "package point of view", also I didn't tried yet but we can probably test it even using directly the nodejs loader and pre-populating the node global with the expected "chrome" global)

Another additional scenario that we need to add a test for is the "AMD module"-mode using RequireJS.

I would be also better to integrate the new tests into the test suite.

In the meantime, I'm assigning this review to Kris to get the final feedback on the currently debated points from this change.

[josephfrazier]

I'm not sure that it is really necessary to test the "commonjs bundled module"-mode with both browserify and webpack (there should be no difference between the two tools from this "package point of view", also I didn't tried yet but we can probably test it even using directly the nodejs loader and pre-populating the node global with the expected "chrome" global)

Good point, we should assume that bundlers work properly, and instead test that their input is correct (like you're suggesting with nodejs tests). I gave this a shot and pushed up commits dd0c355 and 99faff9 to show you all what I came up with. Let me know if it looks like the right direction.

Another additional scenario that we need to add a test for is the "AMD module"-mode using RequireJS.

I've thought a little bit about this, but I'm not too familiar with AMD, so I'm not sure if it makes more sense to try to integrate AMD tests with the jsdom tests, or if it would be easier to make more node-style tests like the above, but use the amdefine or requirejs packages to allow AMD to work in Node. See here for more info: http://requirejs.org/docs/node.html

[rpl]

Merged as f9248e6 (with minor tweaks applied by 31d778c, mostly the same minor changes described in the comments from the last review).

[rpl]

@josephfrazier Thank you so much for this, and my sincere apologies for the long waiting time.

During the last week, Kris and I have been finally able to have a brief chat about this PR, and we agreed on the changes to apply on it before landing it in the master branch.

Given how much time you have been waiting, I opted to apply the needed changes on top of your patches as 31d778c, while I squashed all the patches from this PR in a single commit (f9248e6), with the git metadata preserved (in particular the author, you ;-), and the commit description).

[josephfrazier]

@rpl Glad to see this finally land! Since it's still not possible to install directly from github, I took the liberty of publishing the current master branch (96cfb9c) to NPM as webextension-polyfill. I also added you as an owner of the package, so feel free to take it over from here :)

EDIT: This doesn't work with webpack. See #34