fix(kitsu-core): build list queryparams

[pedep]

I'm not sure this is the way you would prefer to serialize arrays by default, but i personally think its better than the current behaviour.

Expected behaviour:

query serializes arrays the same way a regular <select multiple> would.`

e.g. both

<select name="filter[id_in][]" multiple>
  <option value="1" selected> New York </option>
  <option value="2" selected> Boston </option>
</select>

and

query({filter: {id_in: [1,2]}})

should result in the following query

filter[id_in][]=1&filter[id_in][]=2
# deserialized in Rails: {"filter"=>{"id_in"=>["1", "2"]}}

Current behaviour:

query({filter: {id_in: [1,2]}})

results in this query:

filter[id_in][0]=1&filter[id_in][1]=2
# deserialized in Rails: {"filter"=>{"id_in"=>{"0"=>"1", "1"=>"2"}}}

[pedep]

I'm unsure if its proper for the library to include [] at the end of the key, or if its better to expect the user to include it at the end of the key themself like this query({filter: {"id_in[]": [1,2,3] }})

On one hand many backends use [] at the end to denote an array value (PHP, Rails and more) so its a pretty sane choice, but on the other hand this makes the library more opinionated.

I'm using Rails, so opinionated is fine with me, but i think the decision should be yours @wopian

[wopian]

I think we should go with the unopinionated route to match the JSON:API specs which accepts whatever - so API server libraries in various languages could use any of the formats the specification lists as potentially possible for query parameter filters (filter[x], filter[x][] etc.,).

If the API a user of kitsu/kitsu-core uses the filter[x][] format they'll know to include [] from that API's own documentation. 👍

[wopian]

https://jsonapi.org/format/#query-parameters-families

For example, the filter query parameter family includes parameters named: filter, filter[x], filter[], filter[x][], filter[][], filter[x][y]

[codeclimate]

Code Climate has analyzed commit a9266c7 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (100% is the threshold).

This pull request will bring the total coverage in the repository to 100.0% (0.0% change).

View more on Code Climate.

[wopian]

Released in 10.0.3 (2022-10-30)

[bglimepoint]

Just wanted to add a note that to get the Rails-like convention (filter[id_in][]=1&filter[id_in][]=2) the name of the field needs to be query({filter: {'id_in][': [1,2]}}), which makes sense, but took a little bit to discover :-D

Thank you both for this, it's a great help :-) Just thought the note might be useful for others :-)

[wopian]

query({filter: {'id_in][': [1,2]}})

Is this a typo or are you saying you need to flip the square bracket? If it's the latter, thats a weird bug 🤔

E.g

- query({filter: {'id_in][': [1,2]}}) # comment
+ query({filter: {'id_in[]': [1,2]}}) # expected syntax

[bglimepoint]

We needed to have the "buggy" version for it to produce filter[id_in][] - I can understand why though, as it is filter[{{key string here}}] (I assume) so that meant we got id_in[]: filter[id_in[]] whereas id_in][ meant we closed the first [] pair and then reopened the final one, leading to filter[id_in][] which we wanted; i.e.

diff --git a/packages/kitsu-core/src/query/index.spec.js b/packages/kitsu-core/src/query/index.spec.js
index f736fad..f1d2ee9 100644
--- a/packages/kitsu-core/src/query/index.spec.js
+++ b/packages/kitsu-core/src/query/index.spec.js
@@ -68,6 +68,24 @@ describe('kitsu-core', () => {
       })).toEqual('filter%5Bid_in%5D=1&filter%5Bid_in%5D=2&filter%5Bid_in%5D=3')
     })
 
+    it.skip('builds list parameters with the `filter[xyz_in][]` convention', () => {
+      expect.assertions(1)
+      expect(query({
+        filter: {
+          'id_in[]': [ 1, 2, 3 ]
+        }
+      })).toEqual('filter%5Bid_in%5D%5B%5D=1&filter%5Bid_in%5D%5B%5D=2&filter%5Bid_in%5D%5B%5D=3')
+    })
+
+    it('builds list parameters with the `filter[xyz_in][]` convention, with the `][` syntax', () => {
+      expect.assertions(1)
+      expect(query({
+        filter: {
+          'id_in][': [ 1, 2, 3 ]
+        }
+      })).toEqual('filter%5Bid_in%5D%5B%5D=1&filter%5Bid_in%5D%5B%5D=2&filter%5Bid_in%5D%5B%5D=3')
+    })
+
     it('builds nested list parameters', () => {
       expect.assertions(1)
       expect(query({

It's the most flexible because it supports filter[id_eq][] or filter[id_eq[]], I guess? But was a tad confusing initially :-D

[pedep]

It's the most flexible because it supports filter[id_eq][] or filter[id_eq[]], I guess? But was a tad confusing initially :-D

Exactly, any other implementation will be opinionated towards a certain way of implementing queryParam lists, which we decided against in the earlier thread.

The issue primarily stems from https://github.com/wopian/kitsu/pull/781/files#diff-e5b7f7807be6441d00dd3551357769b805639b0436391e09ab9a83298e49cacbR39 hardcoding the [ and ].

To return to the earlier discussion about convention, i think it would be fine to use a more opinionated serializer.
Even jQuery is doing it now, with a switch to use the old style

http://api.jquery.com/jQuery.param/

As of jQuery 1.4 (2010), the $.param() method serializes deep objects recursively to accommodate modern scripting languages and frameworks such as PHP and Ruby on Rails. You can disable this functionality globally by setting jQuery.ajaxSettings.traditional = true;

As of jQuery 3.0 (2016), the \$.param() method no longer uses jQuery.ajaxSettings.traditional as its default setting and will default to false. For best compatibility across versions, call $.param() with an explicit value for the second argument and do not use defaults.

I don't know if i'm interpreting the RFC wrong, or if i'm looking in the wrong places, but it seems like we're already "breaking" convention by using the parent[child]=value convention.
https://www.rfc-editor.org/rfc/rfc3986#section-3.4

The query component contains non-hierarchical data

If i were to suggest a fix for this now, i would probably lean toward the jQuery approach. Provide a solution which works in 95% of instances, and provide a "bring-your-own-serializer" approach for the rest. This approach only applies to using kitsu proper, since you simply use your own serializer in place of query when using kitsu-core on its own.

It could be as simple as allowing string inputs

export function query (params, prefix = null) {
+ if (typeof params == "string") {
+   return params;
+ }
+
  const str = []
  
  for (const param in params) {
    str.push(
      queryFormat(params[param], prefix ? `${prefix}[${param}]` : param)
    )
  }
  return str.join('&')
}

Which allows for using custom serializers when using kitsu

function myCustomSerializer(obj: any): string;
kitsu.get('bla', { params: myCustomSerializer(paramObj) })

Another solution could be use allow configuration directly in the Kitsu class, like is done with camel and resCase

// https://github.com/wopian/kitsu/blob/master/packages/kitsu/src/index.js
  constructor (options = {}) {
+   if (typeof options.query === 'function') this.query = options.query
+   else this.query = query // or throw an error to let user know
    if (options.camelCaseTypes === false) this.camel = s => s
    else this.camel = camel

  // snip..

      const { data, headers: responseHeaders } = await this.axios.get(url, {
        headers,
        params,
-        paramsSerializer: /* istanbul ignore next */ p => query(p),
+        paramsSerializer: /* istanbul ignore next */ p => this.query(p),
        ...config.axiosOptions
      })

function myCustomSerializer(obj: any): string;
kitsu = new Kitsu({query: myCustomSerializer});

Or maybe just a boolean switch like in jQuery, to swap between pre-written serializers

// https://github.com/wopian/kitsu/blob/master/packages/kitsu/src/index.js
  constructor (options = {}) {
+   this.query = (obj) => query(obj, null, options.traditionalQuery || false)
    if (options.camelCaseTypes === false) this.camel = s => s
    else this.camel = camel
 
  // snip..

      const { data, headers: responseHeaders } = await this.axios.get(url, {
        headers,
        params,
-        paramsSerializer: /* istanbul ignore next */ p => query(p),
+        paramsSerializer: /* istanbul ignore next */ p => this.query(p),
        ...config.axiosOptions
      })


// https://github.com/wopian/kitsu/blob/master/packages/kitsu-core/src/query/index.js
- function queryFormat (value, key) {
+ function traditionalQueryFormat (value, key) {
  if (value !== null && Array.isArray(value)) return value.map(v => queryFormat(v, `${key}[]`)).join('&')
  
// snip..

+ function modernQueryFormat (value, key) {
+   if (value !== null && Array.isArray(value)) return value.map(v => queryFormat(v, `${key}[]`)).join('&')
+   else if (value !== null && typeof value === 'object') return query(value, key)
+   else return encodeURIComponent(key) + '=' + encodeURIComponent(value)
+ }

- export function query (params, prefix = null) {
+ export function query (params, prefix = null, traditionalQuery = false) {
  const str = []

+ queryFormat = traditionalQuery ? traditionalQueryFormat : modernQueryFormat;
  for (const param in params) {
    str.push(
      queryFormat(params[param], prefix ? `${prefix}[${param}]` : param)
    )
  }
  return str.join('&')
}

I'd like @wopian 's two cents on what to do before i spend the time writing the specs and docs, since a few of the above approaches change the default behaviour, which are breaking changes that would probably require at least a minor version bump.

[wopian]

If we check the key ends in [] we should be able to support both cases without running into any major issues I think 🤔

Though I do like the idea for a modern/traditional toggle if that is not feasible. I would recommend defaulting to traditional so it could be released in v10 as an opt-in feature than a breaking change.

If we do do that, the TypeScript rewrite can change default to modern as it itself is going to be a new major.

[pedep]

If we check the key ends in [] we should be able to support both cases without running into any major issues I think thinking

I guess you're thinking something like

+export function paramKeyName (param) {
+  if ([ '[]', '][' ].includes(param.slice(-2))) {
+    return `[${param.slice(0, -2)}][]`
+  }
+
+  return `[${param}]`
+}
+
 export function query (params, prefix = null) {
   const str = []

   for (const param in params) {
     str.push(
-      queryFormat(params[param], prefix ? `${prefix}[${param}]` : param)
+      queryFormat(params[param], prefix ? `${prefix}${paramKeyName(param)}` : param)
     )
   }

in addition to the solution with traditionalQueryFormat and modernQueryFormat, but with the default value changed to traditionalQuery = true for backwards compatability?

[wopian]

in addition to the solution with traditionalQueryFormat and modernQueryFormat, but with the default value changed to traditionalQuery = true for backwards compatability?

Yep!

[wopian]

@bglimepoint try 10.1.0 😀

[bglimepoint]

Thanks @wopian and @pedep!

Just confirming - and I expect that it's just that it's still building+publishing - I'm not seeing 10.1.0 on npm yet (https://www.npmjs.com/package/kitsu?activeTab=versions), fyi :-)

But will give this a go when it's published, thanks again!

[wopian]

Ah, looks like the npm publish command failed silently on both 10.0.5 and 10.1.0 😅

Will manually publish 10.1.0 shortly.

[wopian]

10.1.0 should be live now

[bglimepoint]

Works perfectly, thanks! :-)