koajs / discussions

KoaJS Discussions
2 stars 2 forks source link

Immutable empty query obj #29

Open kilianc opened 7 years ago

kilianc commented 7 years ago
console.log(ctx.query)
if (!ctx.query.sort) {
  ctx.query.sort = '-createdAt'
}
console.log(ctx.query)

Will yield {} and {} but if I pass anything in qs I correctly get {foo:'bar'} and {foo:'bar', sort: '-createdAt'}

jonathanong commented 7 years ago

i would do it this way:

ctx.query = {
  sort: '-createdAt',
  ...ctx.query,
}

does that work?

fl0w commented 7 years ago

Does object spread work without a transpiler?

It works with 8.4+ according to node.green

kilianc commented 7 years ago

I am on 7.10 so I can't but I can Object.assign:

ctx.query = Object.assign({ sort: '-createdAt' }, ctx.query)

Or, looking at the getters setters behavior:

  if (!ctx.query.sort) {
    ctx.querystring = ctx.querystring || 'sort'
    ctx.query.sort = '-createdAt'
  }

It is still very unnecessary unexpected behavior

likegun commented 7 years ago

Which version of koa do you use and are there any middlewares you use before the code?

kilianc commented 7 years ago

@likegun koa@2.3.0 no middleware

jonathanong commented 6 years ago

PR welcomed

garcia556 commented 6 years ago

Tried to reproduce this using node@7.10 and koa@2.3.0 with no luck so far. Wondering what am I doing wrong:

/app # npm ls
npm info it worked if it ends with ok
npm info using npm@4.2.0
npm info using node@v7.10.1
/app
`-- koa@2.3.0 extraneous

Source code:

const
  http = require("http"),
  Koa = require("koa"),
  app = new Koa(),

  PORT = 12345,
  URL = `http://localhost:${PORT}`;

app.use(ctx => {
  console.log(ctx.query)
  if (!ctx.query.sort)
    ctx.query.sort = "-createdAt"
  console.log(ctx.query)

  ctx.body = "Hello Koa";
});

app.listen(PORT);

http.get(URL, res => {
  res.setEncoding("utf8");
  let body = "";
  res.on("data", data => {
    body += data;
  });
  res.on("end", () => {
    console.log(body);
    process.exit(0);
  });
});

Result:

{}
{ sort: '-createdAt' }
Hello Koa
qianL93 commented 6 years ago

failed to reproduce this with koa@2.4.1

dead-horse commented 6 years ago

https://github.com/koajs/koa/blob/master/lib/request.js#L168 ctx.query will always point to the same object if ctx.querystring is not changed. @kilianc can you try to display more informations like ctx.querystring or ctx._querycache ?

gaomd commented 6 years ago

Failed to reproduce with Koa@2.4.1, with Node.js v7.1.0, 7.10.0 and 7.10.1 (as suggested by the original reporter).

likegun commented 6 years ago

@kilianc Failed to reproduce too.Can you display the full code?

benbabic commented 4 years ago

I've hit a similar issue:

ctx.query = { date: new Date() } 
console.log(ctx.query) // prints { "date": "" }

This is likely because the setter is using querystring which defaults all parameter types (like date) it does not support to an empty string https://nodejs.org/api/querystring.html#querystring_querystring_stringify_obj_sep_eq_options.

The workaround that works is to use Object.assign

Object.assign(ctx.query, { date: new Date() })
console.log(ctx.query) // prints the right value for the date

Koa 2.6.2.

Looking at https://github.com/koajs/koa/blob/ed84ee50da8ae3cd08056f944d061e00d06ed87f/lib/request.js#L172-L187 I guess the goal of the getter is to not parse the querystring unless it's being accessed but is there a reason to have the setter be overridden?

This is tagged as PR welcomed: hat change would you be open to?

jonathanong commented 4 years ago

i'm not sure why you all are trying to overwrite query. i would just make a copy.

ctx.locals = {
  ...ctx.query,
  date: new Date()
}
kilianc commented 4 years ago

@jonathanong the problem here is that if fails silently. Can we Object.freeze or put a proxy around it?

jonathanong commented 4 years ago

I’m okay with that. Wanna make a PR?