path-to-regexp icon indicating copy to clipboard operation
path-to-regexp copied to clipboard

`PathFunction` should require parameter

Open OliverJAsh opened this issue 4 years ago • 8 comments

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 avatar Jun 29 '21 18:06 OliverJAsh

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;

OliverJAsh avatar Jun 29 '21 18:06 OliverJAsh

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

blakeembrey avatar Aug 08 '21 02:08 blakeembrey

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 avatar Aug 27 '21 11:08 kettanaito

@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).

kettanaito avatar Aug 27 '21 11:08 kettanaito

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 avatar Aug 27 '21 22:08 blakeembrey

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?

blakeembrey avatar Aug 27 '21 22:08 blakeembrey

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?

kettanaito avatar Aug 27 '21 22:08 kettanaito

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.

blakeembrey avatar Oct 07 '21 06:10 blakeembrey