doc: Rewrite README.md and move build and contribution details into a separate doc file #147

rpl · 2018-07-11T16:20:38Z

This PR goals is to apply some changes to the README.md file to make it a bit more oriented to the kind of information that may be helpful to an extension developer that just wants to use the library instead of building it from its sources, and move the information about building, testing into a separate CONTRIBUTING.md file.

Here is a link to get a preview of how the README.md is going to look like: https://github.com/rpl/webextension-polyfill/blob/doc/rewrite-readme/README.md

TODO list:

README.md

include a table with the supported browser (and make it clear what makes a browser "officially supported")
make it clear which is the scope of the library (and what is likely to be out of its scope)
make it clear that the polyfill is a NOOP on Firefox (and that issues that are only happening when running on Firefox are likely not a polyfill issue and that should be reported on bugzilla)
include a npm badge
move building info into CONTRIBUTING.md file
provide information about how to install the library
provide information about how to setup the library with module bundlers
add a section for the "known limitation and incompatibilities", where we can list the known issues that we already know about (and eventually link MDN when opportune)
fix missing tabId parameter in browser.tabs.sendMessage API call used in example code snippet (Fix Tested an example, got an error: Expected at least 2 arguments for sendMessage() #113)

CONTRIBUTING.md

add some information about the available test suites and their role
provide information about how to run the test suites (and how to run the integration tests on a single browser from the command line)

Rob--W

Another major incompatibility is the lack of support for callbacks instead of promises. Can you also document this?

The API promises to be a Promise-based API. However, promises are not supported for unrecognized APIs, and callbacks have to be used for them.

The CONTRIBUTING document lacks a description of the expected commit format in pull requests. This is already checked by our linter and CI, so if you want to keep the doc as short as possible, it's OK to omit these details.

Rob--W · 2018-07-11T17:47:42Z

CONTRIBUTING.md

+
+npm install
+
+npm run build


This is redundant, because it will already be run at the npm install call due to the prepublish hook that calls build.

Perhaps add a comment after npm install:

# ^ installs the dependencies and runs "npm run build" to build the polyfill.

prepublish: Run BEFORE the package is packed and published, as well as on local npm install without any arguments. (See below)

https://docs.npmjs.com/misc/scripts

Rob--W · 2018-07-11T17:49:33Z

CONTRIBUTING.md

+
+This will install all the npm dependencies and build both non-minified and minified versions
+of the final library, and output them to `dist/browser-polyfill.js` and `dist/browser-polyfill.min.js`,
+respectively, and finally executes the unit tests on the generated dist files.


s/execute/executes/.

Rob--W · 2018-07-11T17:49:58Z

CONTRIBUTING.md

+
+This project provides two test suites:
+
+- unit tests (which only require nodejs to run)


s/nodejs/Node.js/.

Rob--W · 2018-07-11T17:53:24Z

CONTRIBUTING.md

+
+### Unit Tests
+
+The unit tests are executed into nodejs and use jsdom to create a browser-like environment in which the library is executed and tested.


s/into/in/ (use in for location, into for movement).
s/nodejs/Node.js/

How about this:

The unit tests are run in Node.js with [Mocha](https://mochajs.org/), and use jsdom and [Sinon](http://sinonjs.org/) to mock a browser-like environment for testing the library.

Rob--W · 2018-07-11T18:03:15Z

CONTRIBUTING.md

+
+The unit tests are executed into nodejs and use jsdom to create a browser-like environment in which the library is executed and tested.
+
+The unit tests are useful to achieve an higher code coverage of the tests, and it is easier to inject spies and shims to make assertions on some of the behavior implemented internally.


I think that this paragraph/line can be removed. Increasing test coverage is obvious (first part), and spies/shims are also kind of obvious when one looks at existing code for examples.

For contributors, it would be more helpful if you describe the location of unit tests, i.e. test/test-*.js.

Rob--W · 2018-07-11T18:12:34Z

README.md

+| Edge          | *Not supported* (may become unofficially supported once #114 lands)                                  |
+
+The polyfill is being tested explictly (with automated tests that run on every pull request) on the platforms that are
+**officially supported** last stable versions of Chrome and Firefox.


This sentence does not flow well.

Rob--W · 2018-07-11T18:14:27Z

README.md

+The polyfill is being tested explictly (with automated tests that run on every pull request) on the platforms that are
+**officially supported** last stable versions of Chrome and Firefox.
+
+On Firefox, this library is actually acting as a NO-OP: it detects that is running on Firefox (where the Promise-based


It does not detect that it is running in Firefox. How about:

It detects that the Promise-based APIs are provided (by Firefox) and does not replace the global `browser` object.

Rob--W · 2018-07-11T19:42:41Z

README.md

@@ -84,6 +116,45 @@ browser.tabs.executeScript({file: "content.js"}).then(result => {
 });
 ```

+### Basic Setup with module bundlers
+
+This library is built as an **UMD module** (Universal Module Definition), and so it can also be used with module bundlers (and explictly tested on both **webpack** and **browserify**) or AMD module loaders.


s/an **UMD**/a **UMD**/

Rob--W · 2018-07-11T19:49:22Z

README.md

+"Firefox only" issues should be reported upstream on Bugzilla:
+- https://bugzilla.mozilla.org/enter_bug.cgi?product=WebExtensions&component=Untriaged
+
+### API methods or options that are only avaible when running in Firefox


s/avaible/available/.

Rob--W · 2018-07-11T19:49:31Z

README.md

+
+This section aims to keep track of the most common issues that an extension may have.
+
+### Issues that happens only when running on Firefox


s/happens/happen/.

Rob--W

Nits + one more thing (lack of support for callbacks, link to #102 ).

Another major incompatibility is the lack of support for callbacks instead of promises. Can you also document this?

Rob--W · 2018-07-18T17:52:06Z

CONTRIBUTING.md

+* `chore: Upgraded yargs to 3.x.x`
+
+If you want to use scopes then it would look more like:
+`test(integration): Added test extension for newAmozingAPI`.


Amozing -> Amazing?

Rob--W · 2018-07-18T17:55:01Z

README.md

+| Chrome        | *Officially Supported* (with automated tests)                                                        |
+| Firefox       | *Officially Supported as a NO-OP* (with automated tests for comparison with the behaviors on Chrome) |
+| Opera         | *Unofficially Supported* as a Chrome-compatible target (but not explicitly tested in automation)     |
+| Edge          | *Not supported* (may become unofficially supported once #114 lands)                                  |


This still not a link - please change it to [#114](https://github.com/mozilla/webextension-polyfill/pull/114).

Rob--W

Looks good, still there is a small nit. One that I mentioned before (executes -> execute):
https://github.com/mozilla/webextension-polyfill/pull/147/files/f70fa49f56d8cd5a50669a108c047fecea26f791#r201784084

and another one below.

Feel free to squash and merge after making those changes.

Rob--W · 2018-07-19T10:16:25Z

README.md

@@ -272,6 +272,10 @@ This library tries to minimize the amount of "special handling" that a cross-bro

 This section aims to keep track of the most common issues that an extension may have.

+### No callback supported by the Promise-based APIs on Chrome
+
+While Firefox some of the asynchronous API methods (the ones that return a promise) also support the callback parameter (mostly as a side effect of the backward compatibility with the callback-based APIs available on Chrome), the Promise-based APIs provided by this library do not support the callback parameter (See ["#102 Cannot call browser.storage.local.get with callback"][I-102]).


I think that you accidentally dropped an "in" here. I suggest to reword the start from

While Firefox some of the asynchronous API methods (...) also support

to

While some of the asynchronous API methods in Firefox

… separate doc file

Rob--W reviewed Jul 11, 2018

View reviewed changes

rpl mentioned this pull request Jul 17, 2018

style: Renamed some of the things #145

Closed

Rob--W reviewed Jul 18, 2018

View reviewed changes

Rob--W mentioned this pull request Jul 18, 2018

Bump package version? #150

Closed

Rob--W approved these changes Jul 19, 2018

View reviewed changes

rpl force-pushed the doc/rewrite-readme branch from 452d5db to 2dc0efe Compare July 19, 2018 10:45

doc: Rewrite README.md and move build and contribution details into a…

3b82708

… separate doc file

rpl force-pushed the doc/rewrite-readme branch from 2dc0efe to 3b82708 Compare July 19, 2018 10:53

rpl merged commit 440857e into mozilla:master Jul 19, 2018

rpl deleted the doc/rewrite-readme branch March 15, 2021 10:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: Rewrite README.md and move build and contribution details into a separate doc file #147

doc: Rewrite README.md and move build and contribution details into a separate doc file #147

rpl commented Jul 11, 2018 •

edited

Loading

Rob--W left a comment

Rob--W Jul 11, 2018

Rob--W Jul 11, 2018

Rob--W Jul 11, 2018

Rob--W Jul 11, 2018

Rob--W Jul 11, 2018

Rob--W Jul 11, 2018

Rob--W Jul 11, 2018

Rob--W Jul 11, 2018

Rob--W Jul 11, 2018

Rob--W Jul 11, 2018

Rob--W left a comment

Rob--W Jul 18, 2018

Rob--W Jul 18, 2018

Rob--W left a comment

Rob--W Jul 19, 2018


		This project provides two test suites:

		- unit tests (which only require nodejs to run)


		### Unit Tests

		The unit tests are executed into nodejs and use jsdom to create a browser-like environment in which the library is executed and tested.


		The unit tests are executed into nodejs and use jsdom to create a browser-like environment in which the library is executed and tested.

		The unit tests are useful to achieve an higher code coverage of the tests, and it is easier to inject spies and shims to make assertions on some of the behavior implemented internally.


		This section aims to keep track of the most common issues that an extension may have.

		### Issues that happens only when running on Firefox

doc: Rewrite README.md and move build and contribution details into a separate doc file #147

doc: Rewrite README.md and move build and contribution details into a separate doc file #147

Conversation

rpl commented Jul 11, 2018 • edited Loading

Rob--W left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rob--W left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rob--W left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpl commented Jul 11, 2018 •

edited

Loading