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

PathFunction should require parameter #255

Open
OliverJAsh opened this issue Jun 29, 2021 · 8 comments
Open

PathFunction should require parameter #255

OliverJAsh opened this issue Jun 29, 2021 · 8 comments

Comments

@OliverJAsh
Copy link

Currently PathFunction is typed such that its parameter is optional, however this allows mistakes such as the following:

import * as p from 'path-to-regexp';
const fn = p.compile<{ id: string }>('/users/:id');
fn({ id: 1 }); // error, good
fn({ id: 'a' }); // no error, good
fn(); // no error, very bad! ❗️
@OliverJAsh
Copy link
Author

Proposal:

export declare function compile<P>(str: string, options?: ParseOptions & TokensToFunctionOptions): P extends object ? PathFunction<P> : PathFunctionWithoutParams;
export declare type PathFunctionWithoutParams = () => string;
export declare type PathFunction<P extends object> = (data: P) => string;

@blakeembrey
Copy link
Member

Sounds good to me, I’ll consider this in the next release. No timeline for that release right now though.

@kettanaito
Copy link
Contributor

kettanaito commented Aug 27, 2021

We may also utilize template literal types to infer path parameters from the path string. See the example. With the parameter inference, the developers would no longer have to specify the parameter types explicitly (which still opens room for error, regardless of the type being required or not).

With explicit parameters type

// Impossible state is a matter of human error.
const fn = p.compile<{ id: string }>('/user/:messageId')
fn({ id: 'foo' }) // OK while being an error state

With inferred parameters type

const fn = p.compile('/user/:messageId')
fn({ id: 'foo' }) // ERROR, unknown key "id"
fn({ messageId: 'foo' }) // OK

The downside of such string inference is that it's available only since TypeScript 4.1.

@kettanaito
Copy link
Contributor

@blakeembrey, I'd like to take initiative in implementing this support. Please let me know which direction would you like to take: explicit required path parameters type or implicit inferred (see the justification above).

@blakeembrey
Copy link
Member

I love the idea of being able to infer in typescript, I assumed it might be too difficult though. Let’s make it required and I can create a “next” branch so it can be in the next release and we can still fix bugs in the current release.

@blakeembrey
Copy link
Member

At the very least I think we could do both, potentially use “unknown” for inferred keys and have it there as type safe validation that the interface type is valid?

@kettanaito
Copy link
Contributor

From my initial assessment, it doesn't require much to have parameters inferrence. If you don't mind, I'll open a pull request where I do both and we can discuss it there. Sounds good?

@blakeembrey
Copy link
Member

Sounds great to me, thanks! Sorry for the delay, getting back through my backlog of open source issues now and promise I'll review your PR by the end of next week.

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

No branches or pull requests

3 participants