es6: var -> const or let #2073

msimerson · Sep 2, 2017

Fixes #2065

Changes proposed in this pull request:

replace uses of var with const or let

Checklist:

docs updated
tests updated
Changes updated

Codecov Report

Merging #2073 into master will increase coverage by 0.01%.
The diff coverage is 71.17%.

@@            Coverage Diff             @@
##           master    #2073      +/-   ##
==========================================
+ Coverage   57.65%   57.66%   +0.01%     
==========================================
  Files          31       31              
  Lines        6485     6475      -10     
  Branches     1622     1622              
==========================================
- Hits         3739     3734       -5     
+ Misses       2746     2741       -5

Impacted Files	Coverage Δ
outbound/tls.js	`87.5% <100%> (ø)`	⬆️
outbound/qfile.js	`93.33% <100%> (ø)`	⬆️
outbound/fsync_writestream.js	`70.83% <100%> (ø)`	⬆️
outbound/config.js	`80.76% <100%> (ø)`	⬆️
config.js	`90.27% <100%> (ø)`	⬆️
rfc1869.js	`79.48% <100%> (ø)`	⬆️
host_pool.js	`97.33% <100%> (ø)`	⬆️
line_socket.js	`75% <100%> (ø)`	⬆️
outbound/mx_lookup.js	`7.69% <16.66%> (ø)`	⬆️
dkim.js	`24.14% <23.72%> (+0.06%)`	⬆️
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0ba36e7...45c27e2. Read the comment docs.

codecov · Sep 2, 2017

Codecov Report

Merging #2073 into master will increase coverage by 0.01%.
The diff coverage is 71.17%.

@@            Coverage Diff             @@
##           master    #2073      +/-   ##
==========================================
+ Coverage   57.65%   57.66%   +0.01%     
==========================================
  Files          31       31              
  Lines        6485     6475      -10     
  Branches     1622     1622              
==========================================
- Hits         3739     3734       -5     
+ Misses       2746     2741       -5

Impacted Files	Coverage Δ
outbound/tls.js	`87.5% <100%> (ø)`	⬆️
outbound/qfile.js	`93.33% <100%> (ø)`	⬆️
outbound/fsync_writestream.js	`70.83% <100%> (ø)`	⬆️
outbound/config.js	`80.76% <100%> (ø)`	⬆️
config.js	`90.27% <100%> (ø)`	⬆️
rfc1869.js	`79.48% <100%> (ø)`	⬆️
host_pool.js	`97.33% <100%> (ø)`	⬆️
line_socket.js	`75% <100%> (ø)`	⬆️
outbound/mx_lookup.js	`7.69% <16.66%> (ø)`	⬆️
dkim.js	`24.14% <23.72%> (+0.06%)`	⬆️
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0ba36e7...45c27e2. Read the comment docs.

wow!

endreszabo · Sep 2, 2017

wow!

wow!

😁

It wasn't as bad as I thought. The first couple commits were eslint --fix, which caught the majority of cases. I didn't see a single error made by --fix. What it did do is leave behind quite a few manual changes, but they were straight forward: either hoist the const/let or add block scope to a switch/case with {}.

msimerson · Sep 2, 2017

wow!

😁

It wasn't as bad as I thought. The first couple commits were eslint --fix, which caught the majority of cases. I didn't see a single error made by --fix. What it did do is leave behind quite a few manual changes, but they were straight forward: either hoist the const/let or add block scope to a switch/case with {}.

So what I would like to do, if you agree, is push a quick point release for the outbound fixes before merging this. Thoughts?

…

On Sep 2, 2017, at 7:28 PM, Matt Simerson ***@***.***> wrote: wow! 😁 It wasn't as bad as I thought. The first couple commits were eslint --fix, which caught the majority of cases. I didn't see a single error made by --fix. What it did do is leave behind quite a few manual changes, but they were straight forward: either hoist the const/let or add block scope to a switch/case with {}. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

baudehlo · Sep 3, 2017

So what I would like to do, if you agree, is push a quick point release for the outbound fixes before merging this. Thoughts?

…

On Sep 2, 2017, at 7:28 PM, Matt Simerson ***@***.***> wrote: wow! 😁 It wasn't as bad as I thought. The first couple commits were eslint --fix, which caught the majority of cases. I didn't see a single error made by --fix. What it did do is leave behind quite a few manual changes, but they were straight forward: either hoist the const/let or add block scope to a switch/case with {}. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

push a quick point release for the outbound fixes

Not a big fan, for a couple reasons: quick releases tend to be followed up by quick releases and because a lot of TLS code recently landed. I'd rather get the outstanding PRs (including this) merged and then start a quiet period. We can point folks with outbound issues to test with npm install -g haraka/Haraka#05ab2d5 to exclude this (and subsequent commits). Meanwhile, the brave and intrepid HEAD runners will get time to deploy and shake out any bugs in HEAD during the quiet period.

msimerson · Sep 3, 2017

push a quick point release for the outbound fixes

Not a big fan, for a couple reasons: quick releases tend to be followed up by quick releases and because a lot of TLS code recently landed. I'd rather get the outstanding PRs (including this) merged and then start a quiet period. We can point folks with outbound issues to test with npm install -g haraka/Haraka#05ab2d5 to exclude this (and subsequent commits). Meanwhile, the brave and intrepid HEAD runners will get time to deploy and shake out any bugs in HEAD during the quiet period.

and #2066 would likely get concluded during the quiet period...

msimerson · Sep 3, 2017

and #2066 would likely get concluded during the quiet period...

if you agree, is push a quick point release for the outbound fixes

Also, to be clear, I'm not opposed. If someone were pushing the release, I would not object.

msimerson · Sep 3, 2017

if you agree, is push a quick point release for the outbound fixes

Also, to be clear, I'm not opposed. If someone were pushing the release, I would not object.

KingNoosh · Sep 3, 2017

KingNoosh requested changes Sep 3, 2017

View changes

There's nothing that seems off besides the case statement braces and the function declarations.

The only thing I'd be adamant on is putting the eslint-disable comments at the top of the file so that it's more clear what rules are disabled in the file instead of needing to scroll a couple hundred lines to find out.

KingNoosh · Sep 3, 2017

bin/haraka

@@ -84,17 +85,17 @@ var usage = [
 ].join('\n');


-var listPlugins = function (b, dir) {
+function listPlugins (b, dir) {


I think it'd be much nicer to do const listPlugins = function (b, dir) { instead since we have a ticket at some point to turn stuff into arrow functions if they're possible.

It'd look more consistent to other declarations too in the future.

That can go either way, I have no strong preference. This way required the least significant change. If we're going to go with arrow functions (a topic for another Issue and PR), that will get addressed then. I'm rather keen to keep this PR limited to deprecating var and minimizing the changes required due to const, let, and function not getting hoisted.

Could we at the very least with the functions that used to be declared with a var be declared with a const?

That seems like a lesser change than turning them into pure function declarations.

KingNoosh · Sep 3, 2017

bin/haraka

@@ -84,17 +85,17 @@ var usage = [
 ].join('\n');


-var listPlugins = function (b, dir) {
+function listPlugins (b, dir) {


I think it'd be much nicer to do const listPlugins = function (b, dir) { instead since we have a ticket at some point to turn stuff into arrow functions if they're possible.

It'd look more consistent with other declarations too in the future.

KingNoosh · Sep 3, 2017

bin/haraka

        // Dump MIME structure and decoded body text?
-        var dump_mime_structure = function (body) {
+        function dump_mime_structure (body) {


Keeping declarations consistent :)
const dump_mime_structure = function (body) {

KingNoosh · Sep 3, 2017

connection.js

@@ -906,10 +908,10 @@ Connection.prototype.ehlo_respond = function (retval, msg) {
                self.disconnect();
            });
            break;
-        default:
+        default: {


Are the braces intentional? Not a complaint but it looks weird if we sometimes brace in switch statements?

Yes, read the eslint docs. Remove them and run eslint to see. ;-)

KingNoosh · Sep 3, 2017

connection.js

@@ -1182,39 +1184,40 @@ Connection.prototype.rcpt_respond = function (retval, msg) {
            this.last_rcpt_msg = msg;
            plugins.run_hooks('rcpt_ok', this, rcpt);
            break;
-        default:
+        default: {


Weird brace

The braces establish a block that contains the scoped declarations.

plugins.js

plugins/attachment.js

KingNoosh · Sep 3, 2017

plugins/avg.js

-                case 'scan':
-                    var end_time = Date.now();
-                    var elapsed = end_time - start_time;
+                case 'scan': {


Weird braces

KingNoosh · Sep 3, 2017

plugins/bounce.js

-    }, (plugin.timeout - 1) * 1000);
-
-    var run_cb = function (abort, retval, msg) {
+    function run_cb (abort, retval, msg) {


Keep declaration's consistent

The only thing I'd be adamant on is putting the eslint-disable comments at the top of the file

I strongly disagree. Putting them inside the scope they effect limits them to only the section of code that needs the exception, as well as making it way more obvious why they are needed. (Hey ya'll, this chunk of code right here is an exception to the rule). It's no matter though, because I've removed them all as they were only needed before I found and set prefer-const's ignoreReadBeforeAssign option.

msimerson · Sep 3, 2017

The only thing I'd be adamant on is putting the eslint-disable comments at the top of the file

I strongly disagree. Putting them inside the scope they effect limits them to only the section of code that needs the exception, as well as making it way more obvious why they are needed. (Hey ya'll, this chunk of code right here is an exception to the rule). It's no matter though, because I've removed them all as they were only needed before I found and set prefer-const's ignoreReadBeforeAssign option.

Keep declaration's consistent

I'd like to hear additional opinions. Changing var foo = function () to const foo = function () isn't consistent because const doesn't do variable hoisting like var does. OTOH, switching from a function expression var foo = function () to a function declaration function foo () does hoist the function declaration to the top of its scope. I'd ague that preserving how it works is more consistent than preserving how it looks. Switching to declaration syntax also makes the functions named instead of anonymous (sometimes helpful for debugging) and avoids the temporal dead zone.

References:

msimerson · Sep 4, 2017

Keep declaration's consistent

I'd like to hear additional opinions. Changing var foo = function () to const foo = function () isn't consistent because const doesn't do variable hoisting like var does. OTOH, switching from a function expression var foo = function () to a function declaration function foo () does hoist the function declaration to the top of its scope. I'd ague that preserving how it works is more consistent than preserving how it looks. Switching to declaration syntax also makes the functions named instead of anonymous (sometimes helpful for debugging) and avoids the temporal dead zone.

References:

I'd ague that preserving how it works is more consistent than preserving how it looks.

I agree 100%. This PR is scary enough as it is.

smfreegard · Sep 4, 2017

I'd ague that preserving how it works is more consistent than preserving how it looks.

I agree 100%. This PR is scary enough as it is.

So what I would like to do, if you agree, is push a quick point release for the outbound fixes before merging this. Thoughts?

Personally - I think this is a prudent approach. This pull is massive and whilst it might lint OK, there's always the potential for breakage in such a big change.

We've been waiting for the outbound changes for ages and we've finally got something that we think should work better, so I'd rather get that out so we don't have a broken 'latest' as we currently do

It would be annoying if we pushed out the fixes for outbound only for something else to get broken in the meantime by such a big change. It's not like this pull is going to make a fundamental difference to the performance or bring any new functionality (aside from variable checking). It's just a 'nice to have'.

smfreegard · Sep 4, 2017

So what I would like to do, if you agree, is push a quick point release for the outbound fixes before merging this. Thoughts?

Personally - I think this is a prudent approach. This pull is massive and whilst it might lint OK, there's always the potential for breakage in such a big change.

We've been waiting for the outbound changes for ages and we've finally got something that we think should work better, so I'd rather get that out so we don't have a broken 'latest' as we currently do

It would be annoying if we pushed out the fixes for outbound only for something else to get broken in the meantime by such a big change. It's not like this pull is going to make a fundamental difference to the performance or bring any new functionality (aside from variable checking). It's just a 'nice to have'.

rebased, fixing all the merge conflicts.

msimerson · Sep 5, 2017

rebased, fixing all the merge conflicts.

@baudehlo

rebased again, fixing merge conflicts. Any ETA on that quick release @baudehlo, I'd like to get this landed and start a quiet period.

msimerson · Sep 6, 2017

rebased again, fixing merge conflicts. Any ETA on that quick release @baudehlo, I'd like to get this landed and start a quiet period.

@baudehlo

For @baudehlo's planned quick release, I've created a draft release tagged to HEAD right now. That'll make it easy to draft a release prior to this big scary PR landing. In the meantime, we really ought to get this landed.

msimerson · Sep 10, 2017

For @baudehlo's planned quick release, I've created a draft release tagged to HEAD right now. That'll make it easy to draft a release prior to this big scary PR landing. In the meantime, we really ought to get this landed.

msimerson changed the title from var -> const or let to es6: var -> const or let Sep 2, 2017

msimerson added the ES6 label Sep 2, 2017

msimerson added a commit to msimerson/Haraka that referenced this pull request Sep 2, 2017

revert var -> const/let in lieue of #2073

5279ec8

msimerson added a commit to msimerson/Haraka that referenced this pull request Sep 2, 2017

revert var -> const/let in lieue of #2073

b78568c

msimerson referenced this pull request Sep 2, 2017
Merged
outbound cleanups #2063

msimerson added this to In Progress in ES6 Sep 3, 2017

msimerson added a commit to msimerson/Haraka that referenced this pull request Sep 3, 2017

revert var -> [const|let] to avoid conflicts with #2073

8fd0e3b

msimerson added a commit to msimerson/Haraka that referenced this pull request Sep 3, 2017

revert var -> [const|let] to avoid conflicts with #2073

4efc0de

msimerson added a commit to msimerson/Haraka that referenced this pull request Sep 4, 2017

revert var -> [const|let] to avoid conflicts with #2073

4776a05

Merge branch 'master' into 2065-var-to-const-or-let

Loading status checks…

45c27e2

msimerson merged commit ab969eb into haraka:master Sep 10, 2017
1 of 2 checks passed

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

msimerson deleted the msimerson:2065-var-to-const-or-let branch Sep 10, 2017

haraka/Haraka

Join GitHub today

es6: var -> const or let #2073

Conversation

msimerson commented Sep 2, 2017 • edited

msimerson changed the title from var -> const or let to es6: var -> const or let Sep 2, 2017

This comment has been minimized.

Codecov Report

codecov commented Sep 2, 2017 • edited

Codecov Report

This comment has been minimized.

endreszabo commented Sep 2, 2017

This comment has been minimized.

msimerson commented Sep 2, 2017

msimerson added the ES6 label Sep 2, 2017

msimerson added a commit to msimerson/Haraka that referenced this pull request Sep 2, 2017

msimerson added a commit to msimerson/Haraka that referenced this pull request Sep 2, 2017

msimerson referenced this pull request Sep 2, 2017

msimerson added this to In Progress in ES6 Sep 3, 2017

This comment has been minimized.

baudehlo commented Sep 3, 2017

This comment has been minimized.

msimerson commented Sep 3, 2017

This comment has been minimized.

msimerson commented Sep 3, 2017

This comment has been minimized.

msimerson commented Sep 3, 2017

msimerson added a commit to msimerson/Haraka that referenced this pull request Sep 3, 2017

msimerson added a commit to msimerson/Haraka that referenced this pull request Sep 3, 2017

KingNoosh requested changes Sep 3, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

msimerson commented Sep 3, 2017

This comment has been minimized.

msimerson commented Sep 4, 2017

msimerson added a commit to msimerson/Haraka that referenced this pull request Sep 4, 2017

This comment has been minimized.

smfreegard commented Sep 4, 2017

This comment has been minimized.

smfreegard commented Sep 4, 2017

This comment has been minimized.

msimerson commented Sep 5, 2017

This comment has been minimized.

msimerson commented Sep 6, 2017

msimerson added some commits Sep 1, 2017

This comment has been minimized.

msimerson commented Sep 10, 2017

Hide details View details msimerson merged commit ab969eb into haraka:master Sep 10, 2017 1 of 2 checks passed

1 of 2 checks passed

msimerson deleted the msimerson:2065-var-to-const-or-let branch Sep 10, 2017

msimerson commented Sep 2, 2017 •

edited

codecov commented Sep 2, 2017 •

edited

KingNoosh requested changes Sep 3, 2017

View changes

msimerson merged commit `ab969eb` into haraka:master Sep 10, 2017
1 of 2 checks passed