Es6 const

[tejasmanohar]

PR in response to discussion w/ @jonathanong in issue #484.

I've done large replacements here, all tests pass (before and after using const in tests too). I'm looking through as well to make sure I haven't missed anything... but it's ready for review. This ONLY uses ES6 const and depends on either Node 4.x.x or harmony of our base version 0.12.x.

Question- Should we use let in place of var? We have to be in strict mode if we want to use let w/ Node 4.x.x. There are a couple instances where block-scope and the lack of hoisting is especially useful like w/ for loops (and variations).

[targos]

I'd be careful with let (especially in loops) because it can be bad for performance. See this discussion about it on the Node.js repo.

[tejasmanohar]

@targos In the context of Koa, I'd personally prefer the removal of side-effects of hoisting and var's scoping than the performance implications (from what I can tell based on the thread)... but I guess that's up to the maintainers of this repo- let's make that change another PR if we go forth w/ it. This is consolidated.

[tejasmanohar]

I've PR"d (but not merge- will wait on discussion here first) the usage of let https://github.com/tejasmanohar/koa/pull/1 into the es6_const branch of my fork (which probably is more appropriately named as es6_blockscope if we merge es6_let in). The reason I've done this is because I don't think we should implement

var w/ mutated reference -> let
unless
var w/ static reference -> const

Since let has some performance drawbacks as @targos has mentioned and const provides a lot of readability improvements with little to no drawbacks.

Would love to hear thoughts on this PR (and the PR to my branch adding let) from the community!

// tagging participants from #484
// cc @jonathanong @felixfbecker @ruimarinho @yanickrochon @dead-horse @martysmind

[tejasmanohar]

One issue I'm facing-
When I add 'use strict'; to test/application.js here, one test is failing because

    when an error occurs
      ✓ should emit "error" on the app
      ✓ should respond with 500
      1) should be catchable


  47 passing (142ms)
  1 failing

  1) app.respond when an error occurs should be catchable:
     Error: expected 200 "OK", got 500 "Internal Server Error"

The reason for this is -

it('should be catchable', function(done){
  const app = koa();

  app.use(function *(next){
    try {
      yield next;
      this.body = 'Hello';
    } catch (err) {
      error = err;
      this.body = 'Got error';
    }
  });

  app.use(function *(next){
    throw new Error('boom!');
    this.body = 'Oh no';
  });

  const server = app.listen();

  request(server)
  .get('/')
  .expect(200, 'Got error')
  .end(done);
})

not sure why this is... any ideas? @jonathanong

[felixfbecker]

Maybe this is one of the unnoticed bugs block-scoped variables help you to prevent ;)

[tejasmanohar]

@felixfbecker I don't quite see how this could be an issue of block-scope. error is passed as an argument into this function and is then being mutated. it may be entirely unrelated to the idea I suspected... could be some other case I am not familiar w/ (or am missing). ❓

[felixfbecker]

Ok so I only looked at the snippet you posted but the only thing I see being passed into the catch block is err which is then apparently set to some nonexistent variable error, which is not allowed in strict mode (no global fallback). Or am I wrong?

[tejasmanohar]

Oh that is it man. I double guessed myself. mixed up err and error

[tejasmanohar]

@jonathanong @felixfbecker That issue is now fixed. tejasmanohar@26a10e7. This is the PR adding let for reference, https://github.com/tejasmanohar/koa/pull/1.

I've PR"d (but not merge- will wait on discussion here first) the usage of let https://github.com/tejasmanohar/koa/pull/1 into the es6_const branch of my fork (which probably is more appropriately named as es6_blockscope if we merge es6_let in). The reason I've done this is because I don't think we should implement

var w/ mutated reference -> let
unless
var w/ static reference -> const

Since let has some performance drawbacks as targos has mentioned and const provides a lot of >readability improvements with little to no drawbacks.

Would love to hear thoughts on this PR (and the PR to my branch adding let) from the community!

[felixfbecker]

What is that line even doing? It doesnt seem to have any purpose... Maybe it should be removed if all tests still pass?

[tejasmanohar]

@felixfbecker Tbh, I wondered the same thing. Tests still pass.

I don't see how it could make the tests fail. For example, if error wasn't successfully caught and passed as an argument to that function, wouldn't the value default to undefined and still be assigned without error?

I'm all for removing it if it's purposeless- simple change, but that's something the original authors / maintainers must comment on first.

[felixfbecker]

Who is to git blame on it? :D

[tejasmanohar]

@felixfbecker Not great w/ that... but

$ git blame -L944,+3 test/application.js
2d35cdff (TJ Holowaychuk 2013-11-07 16:15:47 -0800 944)           error = err;
2d35cdff (TJ Holowaychuk 2013-11-07 16:15:47 -0800 945)           this.body = 'Got error';
^9e167c5 (TJ Holowaychuk 2013-08-17 00:15:57 -0700 946)         }

so seems like @tj. there may be a reason behind this, i just don't know it- that's why i've refrained from removing.

[tj]

hmm haha I think that's just an accident from an older version of the test

[tejasmanohar]

@tj ah, alright- maybe it was for testing the value of error caught or something lol. anyhow, submitted a PR - #491 for that to be handled separately (since it's unrelated to let/const)

[jonathanong]

@tejasmanohar can you rebase?

[tejasmanohar]

@jonathanong merged changes from upstream/master. tests still pass, no conflicts.

PS: closed my let PR on my fork for now. const has no significant drawbacks in node 4 and harmony's implementation afaik, but let currently has the performance issues @targos mentioned (though I don't think it's considerable enough to outweigh the side-effects of hoisting)... so if this is merged, I'll suggest that in another PR to koa afterwards and then let the community discuss it out.

[jonathanong]

@tejasmanohar i'm going to merge these into an "es2016" branch

[jonathanong]

f91fd1d!

[tejasmanohar]

@jonathanong perfect. thanks- i will submit a let refactor into the es2015 branch too