Use ES6 classes - v2.x

[macalinao]

This converts Context, Request, and Response to ES6 classes to make things more semantic.

See #597 for the old PR.

[lourd]

Instead of keeping createContext around you could replace where it's used in the http callback with the call to the constructor.

[macalinao]

true. again, did it for reverse compatibility, but it doesn't make sense to keep it, you're right

[jonathanong]

@simplyianm the code as-is won't be merge. do you want to update it based on our comments?

• Ditch all the prototypes
• Make extending the context/request/response objects use class inheritance (which is mostly removing features and adding docs):

app.Context = class Context extends app.Context {
  whatever(){}
}

Anyone have opinions on this signature before @simplyianm or anyone else updates this PR?

[jonathanong]

FWIW i'm -1/2 on this PR because I don't really see any benefits and making it classes just makes it more confusing to me

[macalinao]

I'll update it
On Sun, Jan 17, 2016 at 15:56 jongleberry [email protected] wrote:

FWIW i'm -1/2 on this PR because I don't really see any benefits and
making it classes just makes it more confusing to me

—
Reply to this email directly or view it on GitHub
#605 (comment).

[popomore]

+1

it's easy to extend Application/Context/Request/Response rather than override

[tejasmanohar]

ping @simplyianm

[macalinao]

Fixed @tejasmanohar @jonathanong

[tejasmanohar]

nice! probably best to squash so commit history isn't polluted :P

[tejasmanohar]

ping @jonathanong

[felixfbecker]

The comment Prototype should go since this is not a prototype object anymore, but a class (function) that has a prototype.

[felixfbecker]

This is great imo. It is in line with the new new Koa() syntax, it is easier to understand (especially the getter/setter syntax) and it may make it easier to test koa apps without using supertest by simply creating new contexts / requests / responses.

[jonathanong]

• can we add docs? specifically, how would someone extend the context/request/response with their own methods?
• why are the tests removed? can we rework the tests?

other than that, I'm +0

[macalinao]

I actually disagree with extending the base class -- things like that
should go in the middlewares to make execution more predictable. What are
the benefits to allowing it?
On Fri, Feb 26, 2016 at 11:01 jongleberry [email protected] wrote:

• can we add docs? specifically, how would someone extend the
  context/request/response with their own methods?
• why are the tests removed? can we rework the tests?

other than that, I'm +0

—
Reply to this email directly or view it on GitHub
#605 (comment).

[jonathanong]

well for one, people are using it: #652

middleware is actually an anti-pattern in koa unless it's something you really want to be executed on every request (like checking etags and setting the status as 304 if the body is fresh). everything should be a function that you can just await ex. const body = await parse.json(ctx).

middleware:

• frequently injects globals into your context
• is order dependent, meaning switching middleware order might cause undesirable behaviors or bugs
• usually executes logic on every request, which may be inefficient, or executes logic based on some "filter", which makes it harder to debug

for example, instead of having a body parsing middleware that always sets this.request.body, just const body = await parse.json(ctx). the benefit of this is that sometimes you can do it before authentication and sometimes after, depending on the requirements of the route. changing order like this is difficult with middleware. if done properly, you should be able to see "all" the logic of a route in a single function instead of going through each middleware in different files. ex. you might not want to download a file a user uploaded until they've 1) been authenticated 2) been authorized.

now, back to extending the context. the two reasons for extending context is:

1. allow users to build frameworks on top of koa
2. allow adding functions like parse.json() to context so you don't have to require() a billion things in every file, as well as defining all the globals in one place (vs. multiple middleware).

of course, this is prone to user error, so we never really recommended it strongly, but it's a feature that shouldn't be removed.

[jonathanong]

similar philosophy: vercel/micro#8

koa is the same except we abstract req and res because working with them directly is a pain

[PlasmaPower]

I don't like this PR because it won't fix much given that delegate is still used (multiple inheritance isn't supported in ES6).

[felixfbecker]

@PlasmaPower that is true, but I don't think it has something to do with multiple inheritance. delegate only defines getters that delegate to another object. It could be replaced by writing the getters into the class manually, which would be much better for static analysis and IDEs to understand.

[PlasmaPower]

@felixfbecker could you explain what you mean by "writing the getters into the class manually"?

[felixfbecker]

@PlasmaPower Well instead of doing this

delegate(Context.prototype, 'response')
  .method('attachment')
  .method('redirect')
  .method('remove')
  .method('vary')
  .method('set')
  .method('append')
  .access('status')
  .access('message')
  .access('body')
  .access('length')
  .access('type')
  .access('lastModified')
  .access('etag')
  .getter('headerSent')
  .getter('writable');

you simply write the getters and methods into the class manually:

class Context {

  attachment(filename) {
    this.response.attachment(filename);
  }

  get status() {
    return this.response.status;
  }

  set status(status) {
    this.response.status = status;
  }
}

This is what delegate does. It is a lot more verbose to write it manually. I personally have never been a fan of this delegation because it is not clear for someone who reads the code wehter body delegates to request or response for example, so I always use response / request explicitly anyway. But using delegate here is not a sin, the class is still much cleaner (I think you confused delegate with a mixing function).

[PlasmaPower]

@felixfbecker yeah but that's a pain compared to simply extending Response and Request.

[felixfbecker]

@PlasmaPower Always favor composition over inheritance 😉

[fundon]

Drops delegates module will get better performances.

[jonathanong]

hi, I'm going to close this for now. I don't think we need to move every single thing to ES6 — there's not much benefit in switching to ES6 classes.

thanks!