Skip to content
This repository has been archived by the owner on May 3, 2024. It is now read-only.

fix: PathParams type #4

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix: PathParams type #4

wants to merge 2 commits into from

Conversation

jorroll
Copy link

@jorroll jorroll commented Dec 22, 2021

This pull request fixes the PathParams type.

The example shown in the docs has a type error in Typescript 4.4/4.5.

type Params = PathParams<"/greeting/:firstName/:lastName">;
// => { firstName: string; } & { firstName?: string | undefined; }

The expected type should include lastName: string.

Here's a typescript playground reproduction.

This merge request fixes the issue so that PathParams is properly typed.

As a side note, it's unclear why PathParams is defined as

export declare type PathParams<S extends string> = {
    [P in SplitRoute<S>[number]]: string;
} & {
    [P in SplitRoute<S>[number]]?: string;
};

instead of simply

export declare type PathParams<S extends string> = {
    [P in SplitRoute<S>[number]]: string;
};

Since {someKey: string} & {someKey?: string} is always equivalent to just {someKey: string}.

@jorroll
Copy link
Author

jorroll commented Dec 22, 2021

This was my first time submitting a PR using the new VSCode editor built into Github. Worked well.

@gzuidhof
Copy link
Collaborator

Thank you! I think it's almost there, the only part that is not working now are optional path params, although I'm starting to doubt they ever worked correctly. I think they're not used very frequently. I imagine that is what the [P in SplitRoute<S>[number]]?: string; was for.

@gzuidhof
Copy link
Collaborator

I think CI is failing due to an unrelated reason: we were pinning to a specific version of a library that hadn't been released yet, I can fix that.

@gzuidhof
Copy link
Collaborator

Screenshot 2021-12-22 at 12 28 06

Forgot to add this accompanying screenshot, the param with the question mark should be optional. Happy to merge this PR anyway of course as it's already an improvement :)

@jorroll
Copy link
Author

jorroll commented Dec 24, 2021

I think it's almost there, the only part that is not working now are optional path params, although I'm starting to doubt they ever worked correctly.

Interesting. I wasn't aware of the optional params feature and I didn't realize that so much documentation was behind the link to the path-to-regexp library.

Well modifying this PR to support optional path params should be easy enough. I haven't tested, but I think this code will handle optional params correctly:

/** Split type `"one/:two/:three?/:four"` into `["one", ":two", ":three?", ":four"]` */
type SplitPath<S extends string> =
    string extends S ? string[] :
    S extends `${infer A}/${infer B}` ? [A, ...SplitPath<B>] :
    [S];

/** Convert type `"one" | ":two" | ":three?" | ":four"` into `"two" | "four"` */
type ExtractRequiredParams<S extends string> =
    string extends S ? string :
    S extends `:${infer A}?` ? never :
    S extends `:${infer A}` ? A :
    never;

/** Convert type `"one" | ":two" | ":three?" | ":four"` into `"three"` */
type ExtractOptionalParams<S extends string> =
    string extends S ? string :
    S extends `:${infer A}?` ? A :
    never;

/** 
 * Convert a route path string literal type such as "one/:two/:three?/:four" 
 * into a params interface like `{ two: string; three?: string; four: string }`
 */
export type PathParams<S extends string> = {
    [P in ExtractRequiredParams<SplitPath<S>[number]>]: string;
} & {
    [P in ExtractOptionalParams<SplitPath<S>[number]>]?: string;
};

I'm happy to update this PR with the necessary changes to support optional route params. This being said, having glanced over the path-to-regexp library now, it looks like there are a lot of features in there that aren't currently handled by the PathParams type. It's not clear to me if typescript string literal types are capable of accurately modeling the path-to-regexp options, but considering someone built a SQL database using string type literals, I'm guessing it's possible. I'm not personally interested in trying to flesh out this PR to fully model all the path-to-regexp options though. Are you still interested in merging it?

@gzuidhof
Copy link
Collaborator

gzuidhof commented Dec 24, 2021 via email

@jorroll
Copy link
Author

jorroll commented Jan 3, 2022

(FYI, haven't forgotten about this--just been busy with the holidays. Might be another week or two before I can follow up.)

@jorroll
Copy link
Author

jorroll commented Jan 12, 2022

Ok, I've updated this PR to support optional params in addition to required params. I think it's ready to merge.

Separately, I think it's possible that the Router methods should have their types updated to better support overriding the params (at the moment you might need to cast to unknown first like params as unknown as InterfaceYouWant), but I haven't had time to test yet and it seems like a separate issue.

Aside: I was able to submit and update this PR entirely using Github's new vscode web editor. Pretty nice!

@jorroll
Copy link
Author

jorroll commented Jan 12, 2022

Looks like the test is failing because PathParams doesn't support the + param modifier so I tihnk PathParams<"/post/:path+"> is resulting in the interface { "path+": string }.

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.

2 participants