Skip to content
This repository has been archived by the owner on Jul 14, 2022. It is now read-only.

Support API requests with nested query params #67

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

klall
Copy link

@klall klall commented Mar 19, 2018

When performing a GET request for an individual asset
from the Shopify Admin API I noticed there isn't support for
nested query parameters.

For example performing this request:
GET /admin/themes/#{id}/assets.json?asset[key]=templates/index.liquid&theme_id=828155753
(https://help.shopify.com/api/reference/asset)

results in a query string seen by the server as follows:
{ theme_id: '11949473849',
asset: { key: 'assets/dci_locales.json' } }

However, the library querystring doesn't support nested objects and
therefore the final request URL sent to shopify isn't formed properly.

It works perfectly if I swap out the querystring library with the qs library.

See also nodejs/node-v0.x-archive#1665

When performing a GET request for an individual asset
from the Shopify Admin API I noticed there isn't support for
nested query parameters.

For example performing this request:
GET /admin/themes/#{id}/assets.json?asset[key]=templates/index.liquid&theme_id=828155753
 (https://help.shopify.com/api/reference/asset)

results in a query string seen by the server as follows:
{ theme_id: '11949473849',
  asset: { key: 'assets/dci_locales.json' } }

However, the library querystring doesn't support nested objects and
therefore the final request URL sent to shopify isn't formed properly.

It works perfectly if I swap out the querystring library with the qs library.

See also nodejs/node-v0.x-archive#1665
Copy link
Contributor

@marutypes marutypes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit, but otherwise this makes sense. It'd be nice to add a test for this, currently our test coverage is pretty bad.

If you do both of those I'll be happy to approve / merge.

@@ -1,4 +1,4 @@
const querystring = require('querystring');
const qs = require('qs');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit nicer to still call this querystring since it's much more readable than an acronym.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure no problem. I will also start looking at creating a test for this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing, thank you :)

@inomn inomn mentioned this pull request Jun 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants