Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use const in place of var #11135

Closed
wants to merge 1 commit into from
Closed

Use const in place of var #11135

wants to merge 1 commit into from

Conversation

malted
Copy link
Contributor

@malted malted commented Dec 11, 2021

Summary

Replace the var keyword for the const keyword. The values are not mutated so const is the appropriate initialiser to use.

Motivation

The var keyword is deprecated.

This PR…
-->

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

@malted malted requested a review from a team as a code owner December 11, 2021 12:53
@malted malted requested review from rebloor and removed request for a team December 11, 2021 12:53
@github-actions github-actions bot added Content:Other Any docs not covered by another "Content:" label Content:WebExt WebExtensions docs labels Dec 11, 2021
@github-actions
Copy link
Contributor

@rebloor rebloor requested a review from Rob--W December 13, 2021 16:09
@Rob--W
Copy link
Member

Rob--W commented Dec 13, 2021

var is not incorrect, and is more friendly to developers (for copy-pasting in the devtools and running repeatedly) and testing than const.

Why are you suggesting to replace var with const?

@malted
Copy link
Contributor Author

malted commented Dec 13, 2021

Am I incorrect in thinking the use of var should be discouraged?
My thinking was that since the variables are not updated then they should be declared with const, however if quick developer testing is a concern then at least let should be used. If this is the case I will open a new PR with var replaced with let.

@hamishwillee
Copy link
Collaborator

hamishwillee commented Dec 13, 2021

@Rob--W let and const are more modern javascript that have somewhat safer scoping rules. and const better captures developer intent for invariant values. If the whole of MDN uses var by default then we're not setting a very good example of how code should be written.

To me that outweighs some convenience in using the console for examples etc. I am slightly ambivalent in this particular case because the const doesn't really make much difference when the content can be modified at will.

@wbamberg Perhaps you'd like to comment.

@Rob--W
Copy link
Member

Rob--W commented Dec 13, 2021

The benefit of let and const over var is that it can be used in a block scope without hoisting. Another effect is that it doesn't allow re-declaration.

As briefly mentioned before, I think that for ease of trying out, var suffices here. var is still a valid part of JavaScript.

If there's a desire to communicate an immutable value, const works. Using it in sample code could however give the misleading impression to beginners that it is somehow a constant. So to counter that, let could be a better alternative than const.

Although I mildly prefer var, I won't block against let if there is a broader agreement to do so. In that case, I'd like it to be applied consistently across all documentation (and examples) in the MDN documentation, and not only in a few places.

@wbamberg
Copy link
Collaborator

wbamberg commented Dec 13, 2021

@Rob--W let and const are more modern javascript that have somewhat safer scoping rules. and const better captures developer intent for invariant values. If the whole of MDN uses var by default then we're not setting a very good example of how code should be written.

To me that outweighs some convenience in using the console for examples etc. I am slightly ambivalent in this particular case because the const doesn't really make much difference when the content can be modified at will.

@wbamberg Perhaps you'd like to comment.

Our general guidelines say to avoid var, and to use const rather than let if a variable is not being reassigned: https://developer.mozilla.org/en-US/docs/MDN/Guidelines/Code_guidelines/JavaScript#declaring_variables .

While it's true of course that it's easier to paste var examples into devtools, I don't think this at all makes up for implicitly encouraging people to use var in their code.

That said, I'm not a maintainer for the add-ons docs, and if the maintainers want to adopt different policies that's up to them.

@hamishwillee
Copy link
Collaborator

That said, I'm not a maintainer for the add-ons docs, and if the maintainers want to adopt different policies that's up to them.

@Rob--W So up to you I guess, though this will keep on coming up since to casual contributors the MDN is a monolith.
If it were me I'd encourage/allow let instead of var in this case and also suggest that the changes get made in a few big efforts for easy review rather than spot fixing.

@malted
Copy link
Contributor Author

malted commented Dec 14, 2021

@Rob--W The more modern reference pages such as Array use let for the examples, so the consistency issue you talked about already exists to an extent.

@Rob--W
Copy link
Member

Rob--W commented Dec 15, 2021

That said, I'm not a maintainer for the add-ons docs, and if the maintainers want to adopt different policies that's up to them.

@Rob--W So up to you I guess, though this will keep on coming up since to casual contributors the MDN is a monolith. If it were me I'd encourage/allow let instead of var in this case and also suggest that the changes get made in a few big efforts for easy review rather than spot fixing.

I'm supportive of replacing var with let.

As mentioned before, I'd like consistency across this section of the documentation, which implies that all articles at https://github.com/mdn/content/tree/main/files/en-us/mozilla/add-ons/webextensions and all examples at https://github.com/mdn/webextensions-examples should be rewritten at once. This can be done automatically (e.g. with sed), followed by manual verification afterwards.

@hamishwillee
Copy link
Collaborator

Makes sense. In that case, perhaps we should close this (thanks @ma1ted ) and hand as a job to @rebloor when he has time?

@Rob--W
Copy link
Member

Rob--W commented Dec 17, 2021

Makes sense. In that case, perhaps we should close this (thanks @ma1ted ) and hand as a job to @rebloor when he has time?

Makes sense. It's not a top priority, but if Richard has some time, a (scripted) search-and-replace of var with let + manual verification + PR would be nice.

@malted
Copy link
Contributor Author

malted commented Dec 18, 2021

One thing to consider is that a straight up search and replace might change instances where the var keyword is being talked about, and not in a code block where it is appropriate for replacing. There would be no safe way to do this without reviewing every single replace, as @Rob--W said.

@rebloor
Copy link
Contributor

rebloor commented Dec 18, 2021

Happy to take a look. Given the discussion, I'm assuming it's not the greatest priority we have?

@Rob--W
Copy link
Member

Rob--W commented Dec 19, 2021

Happy to take a look. Given the discussion, I'm assuming it's not the greatest priority we have?

Not the highest priority. I suggest to only do it when you have time to focus on this, so we can quickly go through the review process to minimize the amount of time wasted on resolving merge conflicts.

As mentioned before, an automatic search-and-replace in the /files/en-us/mozilla/add-ons/webextensions/ directory would probably the best way to start. Don't change anything except var to let. Also, do not do a dumb replacement of "var" with "let", because that could result in "variable" becoming "letiable". If you're going to do a regex search-and-replace, then something like this would be the bare minimum: \bvar\b (using a word boundary).

@rebloor
Copy link
Contributor

rebloor commented Dec 20, 2021

I have created Replace var with let in code snippets and examples #11343 and will look at actioning as other priorities permit.

@rebloor rebloor closed this Dec 20, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Content:Other Any docs not covered by another "Content:" label Content:WebExt WebExtensions docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants