Type signature for params is too narrow

[jkepps]

In the code, the params type is defined as follows:

  /**
   * URL search parameters can be provided either as a record object (in which
   * case keys with `undefined` values are ignored) or as an URLSearchParams
   * object. If you want to specify a parameter multiple times, use
   * URLSearchParams with its "array of two-element arrays" constructor form.
   * (The URLSearchParams object is globally available in Node, and provided to
   * TypeScript by @types/node.)
   */
  params?: Record<string, string | undefined> | URLSearchParams;

but in the docs the following example is used:

async getMostViewedMovies(limit = 10) {
    const data = await this.get('movies', {
      params: {
        per_page: limit,
        order_by: 'most_viewed',
      },
    });
    return data.results;
  }

which would fail the type checking because Type number is not assignable to type string.

The params object should be something more like:

params?: Record<string, string | number | boolean | undefined> | URLSearchParams;

[jkepps]

I'm realizing there's a discrepancy between what's on NPM and what's on main in the repo as outlined in #130, but this issue still applies to either case

[glasser]

Hmm. We made our types match the URLSearchParams constructor arguments better in v4/v5, which does mean (at a TS level) that values in the object are expected to be strings, not arbitrary items. And that matches our implementation where we call new URLSearchParams(params); calling new URLSearchParams({per_page: 10}) is a TS error with both the Node and dom implementations (although at runtime it does seem to work with some stringification).

But maybe that was a foolish choice and since this is a convenience parameter we should make it more convenient by calling .toString() for you? I'm not sure. Thoughts @trevor-scheer ?

[jkepps]

For what it's worth, I think it would be fine to leave the type definition as is, there would just need to be an update to the given example in the docs. If it's expected that an instance of URLSearchParams is passed in, that should just be reflected in the example.

I would prefer to just be able to pass in an object where the values are required to be primitives, but could see why that might be too ambiguous from the perspective of the RESTDataSource class. But as you stated, passing non-string primitives in doesn't cause any sort of runtime error, so maybe this is really an issue with the type definition of URLSearchParams in @types/node?

[trevor-scheer]

I'm going to update the docs now for correctness before I release v5, but I think I like the suggestion to extend this type to the primitives which we then stringify for convenience.

Fortunately this change is a nice addition and we can land it as a minor to v5 (which is why I'm deferring for now in favor of a docs change).

Thoughts?