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 support for string union types in searchParams Record value #625

Conversation

milk717
Copy link

@milk717 milk717 commented Aug 29, 2024

Problem Description

I am using swagger-typescript-api to automatically generate API functions, which are customized using an ejs template. The generated API request functions look like this:

import ky from 'ky';

type UserTypeEnum = 'user' | 'admin';

export const getUserList = (params?: { userType?: UserTypeEnum | null }) => {
  ky.get('https://example.com/user', {
    searchParams: params,
  });
};

However, the Record type used in searchParams does not support string union types as values, leading to the following TypeScript error:

TS2322: Type
{   userType?: UserTypeEnum | null | undefined; } | undefined
is not assignable to type SearchParamsOption
Type
{   userType?: UserTypeEnum | null | undefined; }
is not assignable to type SearchParamsOption
Type
{   userType?: UserTypeEnum | null | undefined; }
is not assignable to type Record<string, string | number | boolean>
Property userType is incompatible with index signature.
Type UserTypeEnum | null is not assignable to type string | number | boolean
Type null is not assignable to type string | number | boolean

TypeScript Error Screenshot

As a result, I am forced to cast params using as never to bypass the error, which is not ideal.

Proposed Solution

By adding unknown to the value type of the Record used in searchParams, this TypeScript error can be resolved. This allows for better flexibility when dealing with union types, improving the overall developer experience.

Updated the `SearchParamsOption` type to support string union types as valid values in the Record object, enhancing flexibility and compatibility with various use cases.
@sholladay
Copy link
Collaborator

sholladay commented Aug 29, 2024

Union types are fine. The issue is that your type specifies that it can sometimes be null, which is not allowed. The types for the searchParams option only allow non-null params. So this is working as intended and I don't think unknown is appropriate in this context.

If we did allow null as a param value, there would undoubtedly be disagreement about how to handle it. Some would say we should treat it the same as undefined and thus not send the param. Some would say send it as-is, like foo=null, but that would be treated as the string "null"by plenty of backends.

I would recommend finding a way to be certain you have params before setting them in the options.

@milk717
Copy link
Author

milk717 commented Aug 29, 2024

Union types are fine. The issue is that your type specifies that it can sometimes be null, which is not allowed. The types for the searchParams option only allow non-null params. So this is working as intended and I don't think unknown is appropriate in this context.

If we did allow null as a param value, there would undoubtedly be disagreement about how to handle it. Some would say we should treat it the same as undefined and thus not send the param. Some would say send it as-is, like foo=null, but that would be treated as the string "null"by plenty of backends.

I would recommend finding a way to be certain you have params before setting them in the options.

Ah, I see! I misunderstood the issue initially.

I completely agree with your point that null should not be allowed as a value in the Record. In that case, I’ll look into how I can adjust my EJS template to remove null and only use undefined. I’ll go ahead and close this PR. Thank you for the clarification!

@milk717 milk717 closed this Aug 29, 2024
@milk717 milk717 deleted the chore/enable-string-union-in-searchParams branch August 29, 2024 06:46
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.

2 participants