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

Replace var with let in examples #484

Merged
merged 2 commits into from
Aug 10, 2022
Merged

Conversation

rebloor
Copy link
Collaborator

@rebloor rebloor commented Mar 12, 2022

Summary

This change replaces the use of the var with let in the examples. This addresses the requirement identified in "Replace var with let in code snippets and examples" #11343 (which in turn was originally identified in discussions on "Use const in place of var" #11135).

After completing the global search and replace, a selection of examples was loaded and tested (approximately 10). Only store-collected-images was found to have issues, which were corrected by reverting the replace ofvar with let in store-collected-images/webextension-plain/deps/uuidv4.js

store-collected-images/webextension-plain/deps/uuidv4.js
@rebloor rebloor requested a review from Rob--W March 12, 2022 01:57
@rebloor rebloor self-assigned this Mar 12, 2022
@rebloor rebloor requested a review from rpl June 23, 2022 17:44
Copy link
Member

@rpl rpl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @rebloor, I just took a look (to the diffs, I haven't expanded the context of all of them) and most of them looks ok, only two changes highlighted in the inline review comments below seems something that should be reverted.

@@ -41,7 +41,7 @@ module.exports = {
},
plugins: [
// Since some NodeJS modules expect to be running in Node, it is helpful
// to set this environment var to avoid reference errors.
// to set this environment let to avoid reference errors.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be changed, the inline comment refers to OS environment variables.

@@ -4,7 +4,7 @@
} else if (typeof exports !== "undefined") {
factory(exports);
} else {
var mod = {
let mod = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think anything that is third party dependency for a webextensions-example (as in "coming from an outside repo") should be kept out of this change and left unmodified (and as a side note this part is not even part of that library, it is actually code injected automatically as part of the bundling).

@rebloor
Copy link
Collaborator Author

rebloor commented Jul 10, 2022

@rpl thank you, I have reverted those changes.

@rebloor rebloor requested review from rpl and removed request for Rob--W July 11, 2022 00:05
@rebloor rebloor merged commit 82217a0 into mdn:master Aug 10, 2022
@rebloor rebloor deleted the replace-var-with-let branch August 10, 2022 16:28
@rebloor rebloor mentioned this pull request Oct 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants