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

lib: use const to define constants #541

Closed
wants to merge 1 commit into from
Closed

lib: use const to define constants #541

wants to merge 1 commit into from

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jan 21, 2015

This commit replaces a number of var statements throughout the lib code with const statements.

This commit replaces a number of var statements throughout
the lib code with const statements.
@vkurchatkin
Copy link
Contributor

@cjihrig do you think const should be used instead of var whenever possible? also, what about let?

@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 21, 2015

@vkurchatkin I would be in favor of using const in more places, as well as using let, but that would amount to nearly a full rewrite of all var statements, and a very noisy diff. I tried to hit mostly major items like require() statements and real constants with this commit. I think in the future we could just update the var statements as relevant code is changed. Make sense?

@bnoordhuis
Copy link
Member

Rubber-stamp LGTM assuming the CI is happy.

I personally wouldn't object to a PR that let/const-ifies all the things, noisy though it may be. I'm sure it will shake out a few bugs.

@wavded
Copy link
Contributor

wavded commented Jan 21, 2015

Just to throw this in the mix, it may be worth running some benchmarks on this change. We had some code that utilized const a bit back that a caused us a slow down, when we set it back to var it was fine. All that said, things may be fine now (was about a year or so ago).

@bnoordhuis
Copy link
Member

@wavded I checked that earlier this week for another PR. I'm reasonably sure that using const and let should be fine now as long as you stick to strict mode.

@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 21, 2015

@bnoordhuis what's your take on the CI? There is some red, but it seems to be on things that have fluctuated in recent builds.

@wavded
Copy link
Contributor

wavded commented Jan 21, 2015

good to know, thx @bnoordhuis

@rvagg
Copy link
Member

rvagg commented Jan 21, 2015

@cjihrig: re CI: there's only one or two persistently failing tests, I can't name them off the top of my head. You should ignore FreeBSD for now, that's still WIP and the Windows MSVS 2015 one can be ignored too and will be removed shortly. The other ones are important to pay attention to.

@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 21, 2015

@rvagg Ignoring FreeBSD and Windows MSVS 2015, that leaves test-vm-timeout failing on Ubuntu. It appears to have failed as recently as two builds ago.

@bnoordhuis
Copy link
Member

@cjihrig This PR is good to go. The parallel/test-vm-timeout failure is unrelated.

cjihrig added a commit that referenced this pull request Jan 21, 2015
This commit replaces a number of var statements throughout
the lib code with const statements.

PR-URL: #541
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 21, 2015

Thanks! Landed in 804e7aa

@cjihrig cjihrig closed this Jan 21, 2015
@cjihrig cjihrig deleted the consts branch January 21, 2015 21:23
@mgol
Copy link
Contributor

mgol commented Feb 4, 2015

I personally wouldn't object to a PR that let/const-ifies all the things, noisy though it may be. I'm sure it will shake out a few bugs.

@bnoordhuis If that's still true, I might help. :) I've never contributed to Node.js but I've worked for the past 2 years on a purely let/const codebase; besides, going through the files will make me a little familiar with the project, opening the way for potential future contributions.

We might even blacklist var with a linter if that's OK by the committee.

I know @chrisdickinson is working on switching the linting to ESLint; that might generate a diff touching most JS files at least a little so perhaps it'd be best to wait until that happens?

@bnoordhuis
Copy link
Member

@mzgol It recently transpired that let/const still blocks some optimizations in the version of V8 that ships with io.js. I'm afraid that we'll have to take a conservative approach for now. Hopefully the next upgrade will fix that.

@mgol
Copy link
Contributor

mgol commented Feb 8, 2015

Sure, in that case it's better to wait a little.

Michał Gołębiowski

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.

6 participants