Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add qs options #1594

Merged
merged 4 commits into from
Nov 21, 2019
Merged

Add qs options #1594

merged 4 commits into from
Nov 21, 2019

Conversation

DaddyWarbucks
Copy link
Member

Summary

  • [ x] Tell us about the problem your pull request is solving.
    This change allows the client to pass options to the underlying qs library that parses query
    objects into query strings. I initially discovered the need for this as I want to parse null differently than an empty string ''. QS by default treats these as the same, but you can pass an option strictNullHandling to handle that differently.

  • [ x] Are there any open issues that are related to this?
    Not to my knowledge.

  • [ x] Is this PR dependent on PRs in other repos?
    Not to my knowledge

Other Information

I wasn't sure if this is something that should be tested?

@DaddyWarbucks
Copy link
Member Author

DaddyWarbucks commented Oct 1, 2019

In hindsight, it may be better to create a new option called stringifyQuery (open to suggestions on names) that takes a function.
Then it is left to the developer to use any parser they want, but fallback to basic qs if none is added.

{ stringifyQuery: query => qs.stringify(query, { strictNullHandling: true  }) }

...
const queryString =  this.options.stringifyQuery
  ? this.options.stringifyQuery(params)
  :  query.stringify(params);

@daffl
Copy link
Member

daffl commented Oct 26, 2019

Yes, I think a getQuery method that can be overriden makes the most sense so something like this:

makeUrl (params, id) {
  params = params || {};
  let url = this.base;

  if (typeof id !== 'undefined' && id !== null) {
    url += `/${encodeURIComponent(id)}`;
  }

  return url + this.getQuery(params);
}

getQuery (params) {
  if (Object.keys(params).length !== 0) {
    const queryString = query.stringify(params);

    return `?${queryString}`;
  } 

  return '';
}

@daffl daffl merged commit 5f21272 into feathersjs:master Nov 21, 2019
@bertho-zero
Copy link
Collaborator

The addQueryPrefix option seems to meet the need:

getQuery(params) {
  return qs.stringify(params, { addQueryPrefix: true });
}
qs.stringify({ a: 'b', c: 'd' }, { addQueryPrefix: true }) // "?a=b&c=d"
qs.stringify({}, { addQueryPrefix: true }) // ""

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants