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

Request / response aliases are bad idea #849

Open
ivan-kleshnin opened this Issue Nov 3, 2016 · 26 comments

Comments

@ivan-kleshnin
Copy link
Contributor

ivan-kleshnin commented Nov 3, 2016

I suspect I'll met a disagreement but I have to say it.
Request / Response aliases are very confusing. And I'm not a Koa newcomer.

Why this.body is this.response.body while this.headers are this.request.headers?
this.request.body is used often as well as this.response.headers. Oh, wait, the latter is this.set(...)! Did I tell you it's confusing?

I know, it's optional and I may use full path (as I do). But, the thing is, the library
authors are following the "convention" and create first shortcut-ful then even shortcut-only APIs. Like this.path without a proper copy in this.request.path.

I feel like way too many things are in the single namespace right now.

Koa is mature and established. Why I bother?
Because async-await is in the Node 7.0 and I expect more people will start to adopt this framework soon. Maybe Koa 2 could take an opportunity to clean this up.

Memorizing these tables is not a friendly option

Request aliases

The following accessors and alias Request equivalents:

ctx.header
ctx.headers
ctx.method
ctx.method=
ctx.url
ctx.url=
ctx.originalUrl
ctx.origin
ctx.href
ctx.path
ctx.path=
ctx.query
ctx.query=
ctx.querystring
ctx.querystring=
ctx.host
ctx.hostname
ctx.fresh
ctx.stale
ctx.socket
ctx.protocol
ctx.secure
ctx.ip
ctx.ips
ctx.subdomains
ctx.is()
ctx.accepts()
ctx.acceptsEncodings()
ctx.acceptsCharsets()
ctx.acceptsLanguages()
ctx.get()

Response aliases

The following accessors and alias Response equivalents:

ctx.body
ctx.body=
ctx.status
ctx.status=
ctx.message
ctx.message=
ctx.length=
ctx.length
ctx.type=
ctx.type
ctx.headerSent
ctx.redirect()
ctx.attachment()
ctx.set()
ctx.append()
ctx.remove()
ctx.lastModified=
ctx.etag=

I guess this.request.foobar felt too long to write and you probably didn't want to get this.req confused with native Node Request which is commonly called req. But still this is not worth the encomplication IMO.

@dominhhai

This comment has been minimized.

Copy link

dominhhai commented Nov 3, 2016

It makes me confused too.
Sometimes, have to check the API to use them.

@nathanloisel

This comment has been minimized.

Copy link

nathanloisel commented Nov 3, 2016

totally agree, I stop to use ctx.body ect...
Newcomers on my koa projects are always lost with that kind of aliases.

Maybe ctx.req.x/ctx.res.x or cyx.in.x/ctx.out.x

@tj

This comment has been minimized.

Copy link
Member

tj commented Nov 3, 2016

Many of these can only be read or write, ctx.method, ctx.path, ctx.status for example don't make sense for a response. A few are definitely ambiguous (ctx.body, ctx.headers), the rest are fine IMO, you only need to memorize 3 or 4.

Even ctx.headers doesn't even really apply since you'd never do ctx.headers['Content-Type'] = foo in Node, they have to be normalized before setting.

I don't disagree that it's potentially confusing in those cases but if you don't like the Koa sugar you can access the request/response directly as you mentioned, just a compromise.

@chanlito

This comment has been minimized.

Copy link

chanlito commented Nov 3, 2016

Been working for awhile with Koa now, but It doesn't seem to confused me at all.

@jonathanong

This comment has been minimized.

Copy link
Member

jonathanong commented Nov 3, 2016

i personally do const { request, response } = this all the time and always try to use those. i try to do the same in open source projects so its easier to understand.

PRs so that the following would be easier to do would be welcomed:

app.use(async ({ request, response }) => {
  await Promise.resolve('something');

  response.body = 'hi';
})
@ShimShamSam

This comment has been minimized.

Copy link

ShimShamSam commented Nov 27, 2016

@jonathanong Isn't this solved in Koa 2.0 since the context is now passed in as the first parameter instead of overriding this?

@fl0w

This comment has been minimized.

Copy link
Member

fl0w commented Nov 27, 2016

@ShimShamSam The example provided is of Koa@2 signature. That's why he can deconstruct the argument (ctx).

@JeffRMoore

This comment has been minimized.

Copy link
Contributor

JeffRMoore commented Mar 2, 2017

As a newcomer, I find the shortcut confusing. I don't think it adds value.

Should I use ctx.type or should I use ctx.response.type?

Since there can be ambiguity on some properties, I decided to prefer the long form.

On a larger team, I would now have to educate the team on that decision and maintain it through code reviews and through team turnover. If I decide not to enforce consistency on the choice, I still have to educate the team that inconsistency is ok.

It is better to not have a choice.

es6 de-structuring offers a much better shortcut. There is less cognitive load
for those coming to Koa from other frameworks who are used to separate request and response parameters.

In submitting documentation PRs, I would have liked to written this example:

app.use(async ({request, response}, next) => {
  if (!request.accepts('xml')) ctx.throw(406);
  await next();
  response.type = 'xml';
  response.body = fs.createReadStream('really_large.xml');
});

This does not work because now I've lost throw in de-structuring.

What drew me to Koa was its minimalistic sensibility. Context aliases seem directly opposed to that.

I'd recommend:

  • go "full delegation" on context in v2.x to support de-structuring
  • update all documentation to use de-structuring instead of aliases (I volunteer to update docs)
  • indicate the aliases are deprecated in v2.x with scheduled removal in 3.x.
  • Move the alias setup to a separate module, required in 2.x, call it yourself in 3.x
@tj

This comment has been minimized.

Copy link
Member

tj commented Mar 2, 2017

👍 I think that's fair, though if we're destructuring we might as well just pass (req, res) in the signature, I'm not a fan of ctx.request.stuff or destructuring always personally.

Ideally node core was more usable out of the box in terms of what Koa introduces so we wouldn't have to touch any of that. It's pretty ugly having to replace them since it doesn't help node form a thriving middleware ecosystem around primitives that everyone agrees on.

Especially now with async/await allowing for more generic middleware that don't use callbacks. Go's net/http is very Koa-like but I think it's a great model of this if anyone wants inspiration.

@fengmk2

This comment has been minimized.

Copy link
Member

fengmk2 commented Mar 3, 2017

I do think ctx is a great abstraction on koa, especially in enterprise development. It's very helpful to make tracer impl in a easy way, most like ThreadLocal in Java.

It can store many information about the current request context and use it in the following process flows. (e.g.: request a down stream PRC with the tracer data)

More detail can find on "Context on egg"

@tj

This comment has been minimized.

Copy link
Member

tj commented Mar 3, 2017

I can definitely agree with the ambiguity aspect. I also think getters/setters (mostly setters) are kind of bad practice, like doing response.type = "text" still warrants double-checking documentation since you may be wrong about its existence, vs a method call.

Koa definitely straddles that line between work-horse and magic, perhaps a bit too much magic.

@fl0w

This comment has been minimized.

Copy link
Member

fl0w commented Mar 3, 2017

@tj considering what you wrote, would a getter/setter function on context which throws/validates on use be a good idea?

app.use(async (ctx, next) => {
  // pseudo fn-name
  ctx.safeGet('typ') // throws
  ctx.safeGet('type') // get from KoaRequest by default
  ctx.safeSet('type', 'application/json') // set on KoaResponse by default
  ctx.safeGet('response', 'typ') // -> throws
  ctx.safeSet('request', 'type', 'hack')
})
@tj

This comment has been minimized.

Copy link
Member

tj commented Mar 3, 2017

Or just req.type and res.type() etc, getters are maybe reasonable if they always have a value, but if they're sometimes null then that's ambiguous as well, they probably shouldn't even be in JS to be honest haha.

@JeffRMoore

This comment has been minimized.

Copy link
Contributor

JeffRMoore commented Mar 5, 2017

Radical.

At first I thought Context didn't have much purpose once it moved from this to a parameter and that (req, res) would represent "least surprise." However, Request and Response don't extend http anyway, so there is always going to be some surprise.

Would (req, res) be worth the turmoil of another signature change? Is that really on the table?

I can imagine down the line, someone opening an issue "Why so much de-structuring?"

I have a hard time imaging someone opening an issue "merge request and response" after a split.

Recap of options

  1. Prefer de-structuring in the docs, use aliases (included or separate) or long form if you don't like de-structuring.
  2. Prefer long form in the docs, use aliases (included or separate) or de-structuring if you don't like long form.
  3. Change signature to (req, res, next).
  4. Prefer aliases in the docs, de-structure or use long form if you don't like them.

The setters take some getting used to, but I don't mind them.

I haven't tried this, but could one do something like this to catch mis-assignment to properties?

if (process.env.NODE_ENV !== 'production') {
  app.use(async (ctx, next) => {
    ctx.freeze();
    ctx.request.freeze();
    ctx.response.freeze();
    await next();
  });
}
@tj

This comment has been minimized.

Copy link
Member

tj commented Mar 5, 2017

Yea I think splitting is nicer than basically forcing the destructuring. Originally I (obviously) wanted this to be the focus so context made a bit more sense there, I still prefer this over ctx since I would never have closures in a route, always await'd calls. having callbacks in the routes is definitely an anti-pattern but hey.

Clarity-wise ctx.body(val) and friends makes the most sense, not as "cute" but whatever. One potential solution is to have a proxy which yells at you if you use the wrong setter/getter haha.... but yeaaaahhh that's just getting weird, or maybe freezing like you mention, I've never actually used freezing, all these features seem weird in JS.

@magicdawn

This comment has been minimized.

Copy link

magicdawn commented Mar 5, 2017

safeGet/safeSet to mustGet/mustSet like MustXxx in golang 😂

@ivan-kleshnin

This comment has been minimized.

Copy link
Contributor Author

ivan-kleshnin commented Mar 6, 2017

I haven't tried this, but could one do something like this to catch mis-assignment to properties?

@JeffRMoore once you start to enforce things like that – you hit the opposite case (when you want to unfreeze stuff) 😄 Why? For example, you're in the middle of a huge code refactoring and you need to do some weird things with variable / header names. Including request mangling...

I remember working in Python:Flask where Request was freezed by default and it was a PITA for corner cases like that.

I've never actually used freezing, all these features seem weird in JS.

👍

@JeffRMoore

This comment has been minimized.

Copy link
Contributor

JeffRMoore commented Mar 6, 2017

Hehe, freezing is weird, but generators and accessors are not? :)

I see the point of .mustGet(field), but .field or .getField() is more friendly to static tools like flowtype, TypeScript, autocomplete, etc.

@tj

This comment has been minimized.

Copy link
Member

tj commented Mar 6, 2017

Accessors are weird, generators are fine, I don't think they add to the list of error-prone JS features personally.

@zh99998

This comment has been minimized.

Copy link

zh99998 commented Mar 8, 2017

if we have any chance to change that (e.g. koa v3), strongly hope to change signature be (request, response, next) .

@nervgh

This comment has been minimized.

Copy link

nervgh commented Mar 8, 2017

Request / response aliases are bad idea

I'm agree. Because we need keep in mind the mapping.
Furthermore, for example, if we parse request params

ctx.params = ctx.request.params = {...}

we should care about two references.

https://en.wikipedia.org/wiki/Single_source_of_truth

For me, ctx by itself is good because I can pass it to another function/middleware very simple.

@tj

This comment has been minimized.

Copy link
Member

tj commented Mar 8, 2017

I still think if Node's core had this useful stuff it would be the best for everyone, then all middleware could easily share the (req, res) signature. Granted you can already do that, you just lose a lot of the convenience.

@JeffRMoore

This comment has been minimized.

Copy link
Contributor

JeffRMoore commented Mar 11, 2017

There is another signature possibility along the lines of eliminating context that offers some additional error detection possibilities.

The request and context objects could be combined with the delegation removed. response is left alone except for getting a duplicate throw and assert and cookies. Then This PR could be used to enable response to be the value of the promise returned by next.

So you'd end up with either

app.use(async (request, next) => {
  const response = await next()
  response.body = 'Hello World'
  return response
});

or

app.use(async (request, next) => {
  const response = await next()
  response.body = 'Hello World'
});

Requiring the return is more verbose, but allows the framework to detect this case where await is left off:

app.use(async (request, next) => { next() })

It doesn't have the familiarity of (req, res) but maybe makes sense given that you have to pay attention to keeping the Promise chain intact anyway.

@DesignByOnyx

This comment has been minimized.

Copy link

DesignByOnyx commented Mar 28, 2017

I can almost guarantee that if you move to (req, res, next) you will get bogged down with "why doesn't this [meant-for-express] middleware work". Having just started with koa, I like the(ctx, next) signature. At the very least, the signature says "hey, this is koa middleware" and disambiguates from connect-style middleware. The ctx is also a great place to expand information that might be shared between middleware (eg. ctx.user). After only several days of testing, the destructuring does not bother me as I don't use request and response "everywhere" as implied earlier in this thread. I have even started doing things like this:

app.use(async (ctx, next) => ctx.user = { name: "Ryan" });
app.use(async ({user, response}, next) => ...)

Going back to the point of this thread, I definitely think the shortcut aliases are confusing. I'd even pull it into a koa-shortcuts module or put it behind a flag so people can turn it on if they want. This way the "shortcut" stuff can stay separate and documented all in one place - allowing users to be in control of their own confusion.

@ianstormtaylor

This comment has been minimized.

Copy link
Contributor

ianstormtaylor commented Nov 2, 2017

I agree that ctx is actually a nice pattern to have. In the Express world since there's no obvious place to put things people augment the req/res objects in non-standard ways, and then you have to dig through all of the existing properties on them to figure out where things are stored.

ctx.request.params isn't bad to type at all, it's explicit. And if it feels too long, you can always destructure. (I honestly destructure in most of my middleware already like @jonathanong.)

So yeah, +1 to this idea. It would be awesome if the shortcuts were deprecated (maybe with logging in NODE_ENV == 'development') and removed in the next major version.

@mchencb

This comment has been minimized.

Copy link

mchencb commented Nov 5, 2018

I found this confusing too. The other one that confuses me is ctx.req v.s. ctx.request (same for response). And the shorter one, which is easier to write, is not the one that is used more often.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.