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

fix: PathParams type - #4

Open
jorroll wants to merge 2 commits into
SunderJS:masterfrom
jorroll:patch-1
Open

fix: PathParams type#4
jorroll wants to merge 2 commits into
SunderJS:masterfrom
jorroll:patch-1

Conversation

@jorroll

@jorroll jorroll commented Dec 22, 2021

Copy link
Copy Markdown

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

jorroll commented Dec 22, 2021

Copy link
Copy Markdown
Author

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

@gzuidhof

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

jorroll commented Dec 24, 2021

Copy link
Copy Markdown
Author

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

gzuidhof commented Dec 24, 2021 via email

Copy link
Copy Markdown
Collaborator

@jorroll

jorroll commented Jan 3, 2022

Copy link
Copy Markdown
Author

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

@jorroll

jorroll commented Jan 12, 2022

Copy link
Copy Markdown
Author

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

jorroll commented Jan 12, 2022

Copy link
Copy Markdown
Author

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