Replace var with let/const


(Daijiro Wachi) #1

Since the code base is using const/let already, it is a good time to start replacing var with const/let. Here is the list:

  • lib/dedupe.js
  • lib/profile.js
  • lib/fetch-package-metadata.js
  • lib/npm.js
  • lib/install/flatten-tree.js
  • lib/install/diff-trees.js
  • lib/install/inflate-bundled.js
  • lib/install/and-ignore-errors.js
  • lib/install/validate-tree.js
  • lib/install/actions.js
  • lib/install/deps.js
  • lib/install/save.js
  • lib/install/exists.js
  • lib/install/update-package-json.js
  • lib/install/report-optional-failure.js
  • lib/install/realize-shrinkwrap-specifier.js
  • lib/install/is-extraneous.js
  • lib/install/and-finish-tracker.js
  • lib/install/copy-tree.js
  • lib/install/check-permissions.js
  • lib/install/validate-args.js
  • lib/install/action/remove.js
  • lib/install/action/preinstall.js
  • lib/install/action/postinstall.js
  • lib/install/action/build.js
  • lib/install/action/install.js
  • lib/install/action/unbuild.js
  • lib/install/action/prepare.js
  • lib/install/action/global-link.js
  • lib/install/action/global-install.js
  • lib/install/action/move.js
  • lib/install/node.js
  • lib/install/and-add-parent-to-errors.js
  • lib/install/inflate-shrinkwrap.js
  • lib/install/writable.js
  • lib/install/is-fs-access-available.js
  • lib/install/decompose-actions.js
  • lib/install/module-staging-path.js
  • lib/install/mutate-into-logical-tree.js
  • lib/install/access-error.js
  • lib/team.js
  • lib/owner.js
  • lib/ls.js
  • lib/visnup.js
  • lib/edit.js
  • lib/docs.js
  • lib/cache.js
  • lib/config/load-cafile.js
  • lib/config/bin-links.js
  • lib/config/get-credentials-by-uri.js
  • lib/config/clear-credentials-by-uri.js
  • lib/config/core.js
  • lib/config/cmd-list.js
  • lib/config/nerf-dart.js
  • lib/config/set-credentials-by-uri.js
  • lib/config/load-uid.js
  • lib/config/load-prefix.js
  • lib/config/defaults.js
  • lib/config/set-user.js
  • lib/access.js
  • lib/bin.js
  • lib/star.js
  • lib/auth/saml.js
  • lib/auth/sso.js
  • lib/auth/oauth.js
  • lib/install-test.js
  • lib/logout.js
  • lib/run-script.js
  • lib/prefix.js
  • lib/deprecate.js
  • lib/doctor/check-files-permission.js
  • lib/doctor/get-latest-npm-version.js
  • lib/doctor/get-git-path.js
  • lib/doctor/check-ping.js
  • lib/doctor/get-latest-nodejs-version.js
  • lib/link.js
  • lib/build.js
  • lib/completion.js
  • lib/utils/metrics-launch.js
  • lib/utils/locker.js
  • lib/utils/warn-deprecated.js
  • lib/utils/is-windows-shell.js
  • lib/utils/escape-exec-path.js
  • lib/utils/module-name.js
  • lib/utils/git.js
  • lib/utils/metrics.js
  • lib/utils/perf.js
  • lib/utils/link.js
  • lib/utils/did-you-mean.js
  • lib/utils/completion/installed-shallow.js
  • lib/utils/completion/installed-deep.js
  • lib/utils/completion/file-completion.js
  • lib/utils/unsupported.js
  • lib/utils/deep-sort-object.js
  • lib/utils/escape-arg.js
  • lib/utils/error-handler.js
  • lib/utils/umask.js
  • lib/utils/package-id.js
  • lib/utils/lifecycle-cmd.js
  • lib/utils/save-stack.js
  • lib/utils/ansi-trim.js
  • lib/utils/output.js
  • lib/utils/no-progress-while-running.js
  • lib/utils/temp-filename.js
  • lib/utils/child-path.js
  • lib/utils/usage.js
  • lib/utils/parse-json.js
  • lib/utils/is-windows-bash.js
  • lib/utils/error-message.js
  • lib/utils/correct-mkdir.js
  • lib/utils/map-to-registry.js
  • lib/utils/pick-manifest-from-registry-metadata.js
  • lib/utils/read-local-package.js
  • lib/utils/gunzip-maybe.js
  • lib/utils/spawn.js
  • lib/utils/gently-rm.js
  • lib/config.js
  • lib/version.js
  • lib/substack.js
  • lib/token.js
  • lib/help.js
  • lib/set.js
  • lib/shrinkwrap.js
  • lib/search/format-package-stream.js
  • lib/search/all-package-search.js
  • lib/search/package-filter.js
  • lib/search/all-package-metadata.js
  • lib/search/esearch.js
  • lib/init.js
  • lib/outdated.js
  • lib/root.js
  • lib/help-search.js
  • lib/xmas.js
  • lib/install.js
  • lib/unbuild.js
  • lib/install-ci-test.js
  • lib/ping.js
  • lib/get.js
  • lib/rebuild.js
  • lib/unpublish.js
  • lib/dist-tag.js
  • lib/bugs.js
  • lib/view.js
  • lib/stars.js
  • lib/whoami.js
  • lib/search.js
  • lib/adduser.js
  • lib/explore.js
  • lib/fetch-package-metadata.md https://github.com/npm/cli/pull/120
  • lib/prune.js
  • lib/repo.js https://github.com/npm/cli/pull/119

(Kat Marchán) #2

We don’t generally accept PRs for stuff like this because they’re relatively low value for the potential impact (for example, merge conflicts with existing PRs, or accidentally making something const when it was modified sometime later).

That said, @iarna and I think we would accept changes like this if it were done one PR per file, and any files modified also happen to have good coverage. We don’t think consistency is high value enough to warrant this kind of impact in one fell swoop or for higher-risk files.


(Daijiro Wachi) #3

Thank you for your comment.

That is right, doing these all by one PR is not acceptable. I remember the moment when we integrated a lint tool into npm for the first time…

From my point of view, I believe consistency has a small, good thing in term of communication. Using one way helps new contributors to get an idea of how to write a patch and they can focus on value. Apparently, it may be acceptable for two of you if the patches have been kept very small according to your comment. Then I keep working on it little by little, and hope the other people will also take some.


(Lars Willighagen) #4

I don’t mind helping out with this (and it also inflates my 24 Pull Requests score :grin:).

@watilde for the consistency, do you think objects/arrays that should be modified but not replaced be consts or lets?


(Daijiro Wachi) #5

Yes, I think so. let/const is to prevent reassignment, and using Object.freeze would take a little bit more time to review if it’s correct at this moment.


(Lars Willighagen) #6

I know about the reassignment, but I’ve heard people saying that using const for objects that should be modified is communicating the wrong message. Whether that’s the case or not, I just wanted to make sure we were on the same page about that.


(Chris Arnesen) #7

For my two cents, “use const unless your code requires a let” is the most widely accepted rule. “const” does not message any kind of freezing of an array or object, not to me or the folks I code with it doesn’t at least.