Remove undefined params from query

[senz]

Old asker behaviour was broken with usage of object-assign. extend library was removing undefined props during assignment. This option brings back old behaviour.

[Flackus]

//cc @ruslankerimov @narqo

[narqo]

@senz could you explain, what kind of problem have you solved?

[senz]

@narqo as said above, extend library https://www.npmjs.com/package/extend that was used in previous versions of asker was dropping undefined props in query: "Undefined properties are not copied. " New package assign https://github.com/nodules/asker/blob/master/lib/asker.js#L241 is not exhibiting such behaviour. I was trying to bring back old behaviour

ie:

options = {
query: {
a: 1,
b: undefined
}
}

old asker formed url like http://…?a=1 new asker forming url like http://…?a=1&b=

[kaero]

I think, it works as designed. A property with the value undefined means "a key with no value", i.e. key=. You can filter a query object in the client code.

[kaero]

Btw, we should add this point to the migration guide.

[senz]

Semantics of undefined can be different. In earlier version it meant that prop is to be dropped. Im trying to achieve smooth transition on new version, instead of rewriting large chunks of code, or forking. You can see that default is new behavior, while allowing to switch the old one.

20 дек 2016 г. 21:25 пользователь "Phillip Kovalev" < notifications@github.com> написал:

Btw, we should add this point to the migration guide.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/nodules/asker/pull/128#issuecomment-268318453, or mute the thread https://github.com/notifications/unsubscribe-auth/AAQ3TD7LHnD-cr5RY8xszvA1Wf8egUoZks5rKB1qgaJpZM4LR9Vq .

[kaero]

Adding a more one option doesn't looks fine solution for me. I think we must accept one behaviour the canonical, and point the another as buggy. Meaningless continuous support of the legacy & crutches is a shit.

So options:

• return previous behavior as bug fix;
• accept current behavior and add a point to the migration guide.

Selected behaviour must be fixed in the readme anyway.

[senz]

Why? You have some restriction on a option count? I dont think so. On the other side, youre allowing your userbase to switch versions without much blood and tears. This is a good point.

20 дек 2016 г. 21:54 пользователь "Phillip Kovalev" < notifications@github.com> написал:

Adding a more one option doesn't looks fine solution for me. I think we must accept one behaviour the canonical, and point the another as buggy. Meaningless continuous support of the legacy & crutches is a shit.

So options:

• return previous behavior as bug fix;
• accept current behavior and add a point to the migration guide.

Any selected behaviour must be fixed in the readme anyway.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/nodules/asker/pull/128#issuecomment-268325774, or mute the thread https://github.com/notifications/unsubscribe-auth/AAQ3TN4H86huTHF0Pjoy6X2r7DFjb7Z7ks5rKCRGgaJpZM4LR9Vq .

[kaero]

Why? You have some restriction on a option count? I dont think so.

The option support code needs to be supported: fixed, tested, ported, moved to next maintainer, e t.c. Less code – less support cost.

On the other side, youre allowing your userbase to switch versions without much blood and tears. This is a good point.

Yep. And to do it we have 2 good enough options:

• revert old behavior
• keep new behavior, but document it.

Adding the proposed option don't solve the migration problem flawlessly: users must add it to the existing codebase if they want to keep the old behavior, i.e. they have options: to add one peace of code or another, i.e. they have no options and must modify their code.

So as we already broke the backward compatibility and some users already moved to the new version, and may be someone already fixed their client code (but doesn't report it here), i think, the best solution for the current users is to keep the current behavior and document it for future.

[kaero]

And finally, i'm against adding options to tweak the considering of some values as an absence of the key. Adding the proposed option creates a precedent for requests of such options like removeZeroNumberParams, removeEmptyStringParams, removeEmptyArrayParams, etc. The undefined is the same valid value as any other value, so why not to add options for other too?

[senz]

I was trying to cover two cases: for those who already adopted new changes, and those who dont. Both cases are covered with tests in this commit, and default can be easily changed with 1 line. Better solution is to fix broken, before more users adopted new code. Just documenting something that is broken is not a solution either.

[narqo]

@senz During the process of 1.x preparation the behaviour provided by Object.assign() was considered as valid, and no project had any issues related to this part during migration. So my question was about the real life issue that you'd faced.

Adding a more one option doesn't look fine solution for me.

I strongly agree with the statement. From my point, an option that changes the process of options parsing is not a well-designed solution.

If the behaviour causes troubles it must be fixed in the major release. Otherwise, I tend to close the PR as "won't fix".

[senz]

Real life issue is that formed url has changed due to described above behaviour, causing huge troubles with backends. Code was accounted with that undefined are removed. There is no need to close pr, I can remove option, if that is the main question.

[kaero]

Are you really needs to pass undefined properties to the asker as a query? Would it been a problem if the change of the behavior was described in the migration guide?

[senz]

Its a problem for a major codebase that we have. Tenth of resources and hundreds of methods.

[kaero]

So why you propose to add an option, which requires to modify you existing codebase in the same way as patching an objects passed as query?

There is no difference in the patching code one way or another:

ask({ query: queryObject }, handler);

to

ask({ query: queryObject, removeUndefinedParams: true }, handler);

or

ask({ query: removeUndefinedParams(queryObject) }, handler);

The only way to avoid changes is rollback to previous behavior without an option, but proposed changes contradicts the desire to don't fix the existing codebase.

[kaero]

Finally you can do it generally without forking or pushing PR, using simple decorator and safe find-and-replace:

asker-noundefined.js

const asker = require('asker');

module.exports = function(req, handler) {
  const q = req.query;
  const query = Object.keys(q).reduce(function(obj, key) {
    if (q[key] !== undefined)
      obj[key] = q[key];
    return obj;
  }, {});

  return asker(Object.assign({}, req, { query }), handler);
}

And replace require('asker') with require('./asker-noundefined').

[kaero]

@ruslankerimov Are you have an opinion about the changed traits of query params with the value undefined?