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

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 }) // ""

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

Successfully merging this pull request may close these issues.

3 participants