docs: Improve JSDoc of `browser‑polyfill.js` functions and types #171

ExE-Boss · 2019-01-17T21:08:34Z

A lot of the current JSDoc is straight up wrong or incomplete.

This corrects it to improve intellisense in VisualStudio Code when developing this polyfill.

ExE-Boss · 2019-01-17T21:10:05Z

src/browser-polyfill.js

+       * @param {function(K):V} createItem
+       *        A function which will be called in order to create the value for any
+       *        key which does not exist, the first time it is accessed. The
+       *        function receives, as its only argument, the key being created.


The createItem function documentation was moved to the correct location.

src/browser-polyfill.js

ExE-Boss · 2019-01-17T21:11:37Z

src/browser-polyfill.js

@@ -109,19 +124,16 @@ if (typeof browser === "undefined" || Object.getPrototypeOf(browser) !== Object.
     *        The name of the method which is being wrapped.
     * @param {object} metadata
     *        Metadata about the method being wrapped.
-     * @param {integer} metadata.minArgs
+     * @param {number} metadata.minArgs


This has to be set to number, as integer doesn’t exist.

ExE-Boss · 2019-01-17T21:12:13Z

src/browser-polyfill.js

     *        The maximum number of arguments which may be passed to the
     *        function. If called with more than this number of arguments, the
     *        wrapper will raise an exception.
-     * @param {integer} metadata.maxResolvedArgs


maxResolvedArgs hasn’t existed for a long time.

src/browser-polyfill.js

ExE-Boss · 2019-01-17T21:14:01Z

src/browser-polyfill.js

+         * promise).
+         *
+         * @param {Promise<any>} promise
+         */


Converted comment to JSDoc.

Rob--W

This PR mixes some useful changes with changes of questionable value. Please make the following changes, and if you strongly disagree, show why the change is necessary anyway. Keeping unnecessary changes minimal makes it easier to use git blame to follow the history of the code.

Some of the changes only change the format of the JSDoc, but since there is no automatic validation of the formatting in the repo, they may go out of date in the future anyway since none of the main developers of this project are using your tools.

.eslintrc

src/browser-polyfill.js

Rob--W · 2019-01-31T15:09:47Z

src/browser-polyfill.js

+       *        A function which will be called in order to create the value for any
+       *        key which does not exist, the first time it is accessed. The
+       *        function receives, as its only argument, the key being created.
+       * @param {ReadonlyArray<[K, V]>} [items]


Remove this @param. It doesn't help (perhaps we should remove the param altogether since it is never used - please do NOT make that change in this commit though).

The @param should be removed. My comment might have been ambiguous, when I said "please do not make that change", I was referring to the removal of the items parameter from the function.

Removing this without also removing the items parameter would violate the valid‑jsdoc ESLint rule.

src/browser-polyfill.js

Rob--W

Another round of review. Please keep in mind that changes themselves need to be generally useful. We won't accept non-idiomatic changes that only exist to support your IDE. More comments = higher maintenance, and without automatic integration, these comments may eventually go out of date again.

src/browser-polyfill.js

Rob--W

The comments are getting in a better shape now. I'm still not too thrilled with the conversion fromProxy<object> and Proxy<function> to generics T and M. Since your goal is apparently to assist automatic parsing of the JSDoc rules, using TypeScript semantics, could you add a verification step to the build system that verifies that the JSDoc is valid? That's the only way to make sure that the work from this PR is not undone in later changes.

If that is not something that you'd like to do, then I would prioritize human readability over machine-readability, and prefer the use of Proxy<...> instead of generics.

Rob--W · 2019-02-06T11:00:31Z

src/browser-polyfill.js

+       *        A function which will be called in order to create the value for any
+       *        key which does not exist, the first time it is accessed. The
+       *        function receives, as its only argument, the key being created.
+       * @param {ReadonlyArray<[K, V]>} [items]


The @param should be removed. My comment might have been ambiguous, when I said "please do not make that change", I was referring to the removal of the items parameter from the function.

src/browser-polyfill.js

ExE-Boss

JSDoc validation is already performed by ESLint:

webextension-polyfill/.eslintrc

Lines 19 to 32 in 949f08c

    
           "valid-jsdoc": [2, { 
        
             "prefer": { 
        
               "return": "returns", 
        
             }, 
        
             "preferType": { 
        
               "Boolean": "boolean", 
        
               "Number": "number", 
        
               "String": "string", 
        
               "bool": "boolean", 
        
             }, 
        
             "requireParamDescription": false, 
        
             "requireReturn": false, 
        
             "requireReturnDescription": false, 
        
           }],

Note that the only way full validity of the JSDoc comments could be enforced would be to migrate the code to TypeScript and then use a direct (esnext to esnext) conversion, but that’s out of scope of this PR.

ExE-Boss · 2019-02-06T16:19:35Z

src/browser-polyfill.js

+       *        A function which will be called in order to create the value for any
+       *        key which does not exist, the first time it is accessed. The
+       *        function receives, as its only argument, the key being created.
+       * @param {ReadonlyArray<[K, V]>} [items]


Removing this without also removing the items parameter would violate the valid‑jsdoc ESLint rule.

Rob--W

If you're not going to add automated verification, how can we verify that the JSDoc is in fact valid (without installing a full-blown IDE...)? The valid-jsdoc eslint rule isn't helping, because it accepts the JSDoc before and after your patch. So if in your eyes the JSDoc was invalid before the patch, then there should be some validator that shows a failing validation before, and a passing validation after the patch.

src/browser-polyfill.js

ExE-Boss · 2019-02-07T20:09:47Z

Technically speaking, JSDoc can’t really be fully semantically validated¹ as it’s just documentation for developers based on the Javadoc syntax.

¹ It can be syntactically validated, which is what the valid‑jsdoc rule does, which ensures that all parameters and return values are documented.

It can however be checked using tsc, but that also checks a bunch of other TypeScript related stuff, and adding that to the pipeline as‑is² would just make the code unreadable without also converting to TypeScript.

² By that I mean using JavaScript and JSDoc to store type information instead of just using TypeScript directly, but as I said before, that’s out of scope of this PR.

That said, I still want to merge this.

Rob--W

Before this PR, we only used very basic JSDoc annotations (with plain types such as function and string, with the most complicated type being Proxy<object>).
In this PR, you introduced new type annotations based on TypeScript. This raises the bar for contributing to the library, because others are now required to understand TypeScript syntax and its types.

function is clearly simpler than function(any):void, but the latter is also more precise in the expected types (unsurprisingly). I am willing to accept slightly more complicated syntax if there are clear benefits. Since you've taken the efforts to open this PR and repeatedly respond to reviews, I assume that you see benefits, such as better integration in your IDE. This integration relies on automated tooling, so there must be a way (automatically or manually) for me to verify that your annotations are "correct".

Types such as ReadonlyArray, PropertyKey and ProxyHandler<T> are only meaningful with TypeScript, and I'm not even sure if there is any JSDoc parser that can make sense of (message, sender: object, sendResponse: (response?: any) => void) => boolean | Promise<any> | void.

Either show a straightforward way to verify that the types are meaningful, or remove the TypeScript-specific syntax. And if you do decide to drop TypeScript, use Proxy<object> and Proxy<function> instead of T and M.

Rob--W · 2019-02-08T12:33:03Z

src/browser-polyfill.js

+     *        The reply.
+     *
+     *        When `reply.__mozWebExtensionPolyfillReject__` is defined and `true`,
+     *        then the reply is a wrapped `Error` object.


Previously you said that using any would cause an error. If that is the case, consider rewording this line to:

then the reply is an object whose `message` property is set to the original error message.

and remove the two @param below.

src/browser-polyfill.js

Zearin · 2020-01-20T17:01:15Z

Would love to see this merged. I like valid JSDoc.

I don’t like TypeScript, however. I don’t think it should be included in JSDoc (unlike the source code is written in TypeScript as well).

docs: Improve JSDoc of browser‑polyfill.js functions and types

d9c0b42

ExE-Boss commented Jan 17, 2019

View reviewed changes

Rob--W requested changes Jan 31, 2019

View reviewed changes

Address review comments

1c79cf1

Rob--W reviewed Feb 4, 2019

View reviewed changes

Address review comments

a6bbb64

ExE-Boss mentioned this pull request Feb 5, 2019

test(devtools): Fix intermittent DevTools API test timeout #177

Merged

Rob--W reviewed Feb 6, 2019

View reviewed changes

ExE-Boss commented Feb 6, 2019

View reviewed changes

Address review comments

f0a9887

Rob--W reviewed Feb 7, 2019

View reviewed changes

src/browser-polyfill.js Outdated Show resolved Hide resolved

ExE-Boss added 2 commits February 7, 2019 20:59

Merge branch 'master' into jsdoc

19e63a5

Address review comments

83f0ae4

Rob--W reviewed Feb 8, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: Improve JSDoc of `browser‑polyfill.js` functions and types #171

docs: Improve JSDoc of `browser‑polyfill.js` functions and types #171

ExE-Boss commented Jan 17, 2019

ExE-Boss Jan 17, 2019

ExE-Boss Jan 17, 2019

ExE-Boss Jan 17, 2019

ExE-Boss Jan 17, 2019

Rob--W left a comment

Rob--W Jan 31, 2019

Rob--W Feb 6, 2019

ExE-Boss Feb 6, 2019 •

edited

Loading

Rob--W left a comment

Rob--W left a comment

Rob--W Feb 6, 2019

ExE-Boss left a comment •

edited

Loading

ExE-Boss Feb 6, 2019 •

edited

Loading

Rob--W left a comment

ExE-Boss commented Feb 7, 2019 •

edited

Loading

Rob--W left a comment

Rob--W Feb 8, 2019

Zearin commented Jan 20, 2020

	"valid-jsdoc": [2, {
	"prefer": {
	"return": "returns",
	},
	"preferType": {
	"Boolean": "boolean",
	"Number": "number",
	"String": "string",
	"bool": "boolean",
	},
	"requireParamDescription": false,
	"requireReturn": false,
	"requireReturnDescription": false,
	}],

docs: Improve JSDoc of browser‑polyfill.js functions and types #171

Are you sure you want to change the base?

docs: Improve JSDoc of browser‑polyfill.js functions and types #171

Conversation

ExE-Boss commented Jan 17, 2019

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

ExE-Boss Feb 6, 2019 • edited Loading

Choose a reason for hiding this comment

Rob--W left a 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

ExE-Boss left a comment • edited Loading

Choose a reason for hiding this comment

ExE-Boss Feb 6, 2019 • edited Loading

Choose a reason for hiding this comment

Rob--W left a comment

Choose a reason for hiding this comment

ExE-Boss commented Feb 7, 2019 • edited Loading

Rob--W left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zearin commented Jan 20, 2020

docs: Improve JSDoc of `browser‑polyfill.js` functions and types #171

docs: Improve JSDoc of `browser‑polyfill.js` functions and types #171

ExE-Boss Feb 6, 2019 •

edited

Loading

ExE-Boss left a comment •

edited

Loading

ExE-Boss Feb 6, 2019 •

edited

Loading

ExE-Boss commented Feb 7, 2019 •

edited

Loading