fix: webextensions: change let to const, or remove assigning when unn…

[rubiesonthesky]

…ecessary

Summary

Change let -> const where applicable. Remove assigning promise to variable.

If these changes are helpful, I can submit more.

Motivation

Supporting details

Related issues

Metadata

• Adds a new document
• Rewrites (or significantly expands) a document
• Fixes a typo, bug, or other error

[Rob--W]

This PR does a lot more than just converting let to const and removing assignments. It almost looks like an automatic code formatter was applied to format code within code blocks.

On let to const - let's not do that. const is a good way to show that a variable is supposed to be immutable, but in the examples we do not necessarily have to signal that. The presence of const can make it a little bit harder to modify a code example for one's own use.

On Removing assignment "when unnecessary":
There are two classes of code blocks here.

1. The first is the "syntax" block. IMO the current dummy variable names are barely useful because they don't show what the type or meaning of the return value, at least not more than what a method name says. Moreover, regular web API documentation does not include the return value either, and defers to the main text instead. So I am supportive of removing let gettingText = from the syntax.
2. The second class of code blocks are examples. In this case, simplicity is important to novice readers. Having a separate variable name can make it easier to read code. This example from your PR has the same result, but the original version is easier to scan:

- let gettingBadgeText = browser.action.getBadgeText({});
- gettingBadgeText.then(gotBadgeText);
+ browser.action.getBadgeText({}).then(gotBadgeText);

This one would be more readable due to the clear separation between method call and chained .then:

browser.action.getBadgeText({})
  .then(gotBadgeText);

One potential issue with the above is that people may not easily recognize that .then is chaining to the code at the previous line.

In a similar vein, I recommend to keep the namespace + method name all on one single line. This example from your PR puts browser.bookmarks and .create on two lines. If someone where to use Ctrl-F bookmarks.create to find copy-pasteable examples, then they would not find this code snippet.

- let createBookmark = browser.bookmarks.create({
+ browser.bookmarks
+  .create({
+    title: "bookmarks.create() on MDN",

A way to solve that is to do something like this:

browser.bookmarks.create(
  { ... }
).then(onCreated);

... but you can imagine that if the { ... } spans multiple lines, that it may become a challenge to figure out where ).then( is attached to. So in this case, a separate variable name can make it easier to read code by those who are not experienced in balancing parentheses.

[rubiesonthesky]

This PR does a lot more than just converting let to const and removing assignments. It almost looks like an automatic code formatter was applied to format code within code blocks.

Yeah, I used prettier to ensure that there are proper indents and spaces inside braces etc. As I have understood it's going to be adopted more widely in the project little bit later. So I let it to decide when to break to new line and when not. That of course leads to loss of fine tuning line-breaks and formatting. (https://developer.mozilla.org/en-US/docs/MDN/Writing_guidelines/Writing_style_guide/Code_style_guide/JavaScript#choosing_a_format)

On let to const - let's not do that. const is a good way to show that a variable is supposed to be immutable, but in the examples we do not necessarily have to signal that. The presence of const can make it a little bit harder to modify a code example for one's own use.

I little bit disagree with this. When I'm using this kind of examples for my own code, I need to change these to const, because in these context they do not really make sense to be let. At least not for me. Also I think, it would be better to promote const usage, because it prevents accidental mutability. But these examples are not inside function or block, so maybe let is better?

Code style guide also says to prefer const (https://developer.mozilla.org/en-US/docs/MDN/Writing_guidelines/Writing_style_guide/Code_style_guide/JavaScript#variable_declarations) but I'm not sure does it apply here then.

On Removing assignment "when unnecessary": There are two classes of code blocks here.

1. The first is the "syntax" block. IMO the current dummy variable names are barely useful because they don't show what the type or meaning of the return value, at least not more than what a method name says. Moreover, regular web API documentation does not include the return value either, and defers to the main text instead. So I am supportive of removing let gettingText = from the syntax.

2. The second class of code blocks are examples. In this case, simplicity is important to novice readers. Having a separate variable name can make it easier to read code. This example from your PR has the same result, but the original version is easier to scan:

- let gettingBadgeText = browser.action.getBadgeText({});
- gettingBadgeText.then(gotBadgeText);
+ browser.action.getBadgeText({}).then(gotBadgeText);

This one would be more readable due to the clear separation between method call and chained .then:

browser.action.getBadgeText({})
  .then(gotBadgeText);

One potential issue with the above is that people may not easily recognize that .then is chaining to the code at the previous line.

I agree that using assigning to variable is good way to show what the returning value will be. But with promises, I'm not so sure it's good practice to promote for newcomers. For me that also looks like gettingBadgeText would be either a function or the value. Not promise. I was confused this kind of examples in other parts of MDN when helping with code modernization project. Not assigning it to own variable but just using the promise with then is clearer for me.

But all that said, I'm happy to revert changes that are not making examples better.

[Josh-Cena]

@Rob--W FYI the code changes here are adhering to our new JS code guidelines: https://developer.mozilla.org/en-US/docs/MDN/Writing_guidelines/Writing_style_guide/Code_style_guide/JavaScript

[Josh-Cena]

Overall LGTM. Wondering if you could revert the syntax box changes (except getting rid of the assignment)? The comments could be totally refactored in another commit and there's no need to format it right now.

[Josh-Cena]

We don't comment the parameter's type everywhere else, but I guess webext is free to have its own convention, or we can do that as a followup

[Josh-Cena]

This is one extra semicolon I caught. There may be others

[rubiesonthesky]

I'll go through these with comb when doing the other changes, to spot extra semicolons. I think they will be easy to spot when reverting the comment formatting.

[Rob--W]

@Rob--W FYI the code changes here are adhering to our new JS code guidelines: https://developer.mozilla.org/en-US/docs/MDN/Writing_guidelines/Writing_style_guide/Code_style_guide/JavaScript

This PR makes multiple changes, and some of them are not explicitly covered by the writing guidelines. Prettier can help with consistently formatted code, but the result is not necessarily the best format for reference documentation. In particular, the case I mentioned above about the namespace being split may be an issue, and it may be more common in extension documentation because extension APIs are under nested namespaces.

To avoid inconsistencies or comprehensibility issues, I have started a discussion with my team. Once that discussion resolves, there should hopefully be more clarity on the desired direction.

Please do not merge yet until I (or another module peer of the WebExtensions component, or our technical writer (@rebloor)) have signed off on the changes.

[rubiesonthesky]

I formatted manually so that the whole chain should be now again in the same row. But if these changes are not wanted, then this PR can be just closed. Or if there are just some part of the changes, maybe it's better to do those changes in another PR with clear goal.