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

Typescript Improvements possible (in v2.8.0-alpha.2) #229

Closed
HansBrende opened this issue Jan 25, 2022 · 4 comments · Fixed by #263
Closed

Typescript Improvements possible (in v2.8.0-alpha.2) #229

HansBrende opened this issue Jan 25, 2022 · 4 comments · Fixed by #263

Comments

@HansBrende
Copy link
Contributor

HansBrende commented Jan 25, 2022

First of all... bravo for making this library. I became a little frustrated with react-router last weekend and started redesigning everything in it... only to come here and find this library, which I find has very similar ideas to my homebrewed stuff! There are just a couple areas in the typings where my library caught some edge cases and yours does not... so I will share some happy paths and sad paths below (The quoted errors are the errors received from my homebaked implementation. Anything with a red ❌ is where the behavior is unexpectedly different). Hopefully this screenshot gives you some ideas:
Screen Shot 2022-01-24 at 7 42 11 PM

@HansBrende HansBrende changed the title Typescript Improvements possible (in v1.8.2-alpha) Typescript Improvements possible (in v2.8.0-alpha.2) Jan 25, 2022
@HansBrende
Copy link
Contributor Author

HansBrende commented Jan 25, 2022

To go into more detail: there are 2 nice typescript errors that occurred in mine that are not occurring in yours:

(1) Dynamic paths (where the path parameters cannot be deduced) I typed as

type DynamicPathParams = {
    readonly [key in string]?: string
}

Rather than:

type DynamicPathParams = { // BAD (even though this is the same behavior of Record<string, string>) ❌
    [key in string]: string
}

My full definition, for reference, was actually conditional based on the type of key present:

// Can be used EITHER as:
//   1. Params<string> (keys are always conditionally present) OR 
//   2. Params<ParamParseKey<PathString>> (keys are unconditionally present if PathString is known)
export type Params<Key extends string = string> = string extends Key ? {
    readonly [key in Key]?: string;
} : {
    readonly [key in Key]: string;
};

type ParamParseKey<PathString extends string> =
    PathString extends `${infer L}/${infer R}` ? ParamParseKey<L> | ParamParseKey<R>
        : PathString extends `:${infer Param}` ? Param
            : PathString extends '*' ? '*' : never

That way (as in the happy path version of the same thing in the screenshot) you have to use the param! typescript syntax to coerce the type from a possible undefined if the route parameter names are not inferable.

(2) Multiple children of a route (which I am assuming are not allowed if one is a function) are not showing as an error. This is probably because of this React typings bug: DefinitelyTyped/DefinitelyTyped#29307
Where anything is allowed to be a ReactNode due to its inclusion of {} | ReactNodeArray in the definition.
I fixed this same problem in my own homebaked project by defining:

type StrictReactNode = ReactChild | StrictReactNode[] | ReactPortal | boolean | null | undefined;

(exactly the same definition of ReactNode except omitting {} from the union) then doing...

export type RouteChildren<PathString extends string> = 
StrictReactNode | ((params: Params<ParamParseKey<PathString>>) => ReactNode);

@cbbfcd
Copy link
Contributor

cbbfcd commented Jan 27, 2022

👍

@molefrog
Copy link
Owner

molefrog commented Nov 2, 2022

Hi @HansBrende, sorry for the late reply (it's been 10 months and I feel sad that I hadn't responded right away).
Thanks for the suggestions!

It seems like 2) has been fixed already and it does give an error now:
image

Regarding 1), I'll see what I can do to fix this.

@HansBrende
Copy link
Contributor Author

@molefrog awesome!

FYI: (2) has been fixed only for React 18 and upwards. So if you do want the correct error on React 17, you could backport their definition from the fix (which is the same definition as mine).

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 a pull request may close this issue.

3 participants