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

Properly handle query params alongside request bodies #30

Merged
merged 2 commits into from Feb 12, 2019

Conversation

Projects
None yet
4 participants
@eliperkins
Copy link
Member

commented Feb 12, 2019

This varies behavior for GET requests versus all other requests, where parameters
for GET requests should be in the query string, whereas other methods should use
the body.

This adds in the query-string module to help us create these queries.

Fixes #29

@eliperkins eliperkins added the wip label Feb 12, 2019

@eliperkins eliperkins changed the title [WIP] Properly handle query params alongside request bodies Properly handle query params alongside request bodies Feb 12, 2019

Correctly handle combining query parameters with body parameters
This varies behavior for GET requests versus all other requests, where parameters
for GET requests should be in the query string, whereas other methods should use
the body.

@eliperkins eliperkins force-pushed the eli/fix-29 branch from c4c45c8 to 16ac33f Feb 12, 2019

@eliperkins

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2019

@j-martin any chance you'd like to review this PR? I know you mentioned that requests in Python didn't seem to exhibit the same behavior, but testing this branch locally with the REPL seems to allow the query you were attempting to run!

~/src/clubhouse/clubhouse-lib eli/fix-29*
❯ node --experimental-repl-await
> const Client = require('./');
undefined
> const client = Client.create('MY_TOKEN');
undefined
> await client.getResource('search/stories', { query: 'owner:eliperkins' });
{ next:
   '/api/beta/search/stories?query=owner%3Aeliperkins&next=4fb8da73a4b90eef85eab94345caedf423cef770~9',
  data:
   [ { entity_type: 'story',
       archived: false,
       created_at: '2018-12-06T17:41:56Z',
       updated_at: '2019-02-12T15:38:53Z',
       id: 48240,
       external_id: null,
...

@eliperkins eliperkins added bug and removed wip labels Feb 12, 2019

import type { RequestFactory } from './types';

require('fetch-everywhere');

This comment has been minimized.

Copy link
@jeremyheiler

jeremyheiler Feb 12, 2019

Member

Curious: why require instead of import?

This comment has been minimized.

Copy link
@eliperkins

eliperkins Feb 12, 2019

Author Member

Because fetch-everywhere executes side-effects by adding globals when requiring it: https://github.com/lucasfeliciano/fetch-everywhere/blob/b472faedba57cff3d5a6d1f2183812ab47a9f387/fetch-npm-node.js#L11-L16

Using import here would/could be tree-shaken or lazily-required, meaning we could execute our code before the side-effect occurred.

tl;dr: side-effects

This comment has been minimized.

Copy link
@jeremyheiler
@j-martin

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2019

@eliperkins yep, that fixed my issue with the search endpoint. Thanks!

@eliperkins eliperkins merged commit 65edbe4 into master Feb 12, 2019

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@eliperkins eliperkins deleted the eli/fix-29 branch Feb 12, 2019

@j-martin j-martin referenced this pull request Feb 21, 2019

Merged

Upgrade clubhouse lib #56

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.