Feat: Improve generated types to include optional types

[alexmccabe]

Improve the generated typescript file to include optional params on KnownRouteParams object, instead of marking everything as optional

[alexmccabe]

@bakerkretzmar Howdy, what do you think about this? It makes knowing the expected params for a route much easier

[bakerkretzmar]

@alexmccabe thanks for doing this! I added optional to params with bindings and tweaked the types a bit to keep them as 'flat' as possible. What I would really love is something like this:

type KnownRouteParamsObject<I extends readonly ParameterInfo[]> = {
    [T in I[number] as T['name']]: T extends { optional: true } ? Routable<T> | undefined : Routable<T>;
} & GenericRouteParamsObject;

But that doesn't actually make those properties optional....

One last question here—the type errors I get when a route() call doesn't have the required params are really weird. Not the end of the world but I'd love something like type { comment: number; } is missing required property 'post', any ideas? 😄

[bakerkretzmar]

Also wondering if we should call this required instead of optional since that's what Ziggy's already mostly calling this in other places.

[alexmccabe]

@bakerkretzmar I tried to do that with the KnownRouteParamsObject, but it just marks them as potentially being undefined, which doesn't allow for not including them. I wonder if I could created an "optional by key" utility type that could do it. That may also improve the output types too.

I can see if I can make the suggested improvements, then will update to use required, I think that is a better naming convention.

[alexmccabe]

So I have managed to get this far, which I think is a bit of an improvement to the end types being flat. ~It seems to work however there is a small error. I'll keep playing.~

What do you think?

[alexmccabe]

In regards to the weird type error given, this is due to the looseness of the RouteParams type, with one of the union members being ParameterValue. When I remove that from the union, the types are much more obvious (though still verbose, given the function overloading).

If I create another function overload, then it behaves as expected:

export default function route<T extends RouteName>(
    name: T,
    params?: ParameterValue | undefined,
    absolute?: boolean,
    config?: Config,
): string;

[alexmccabe]

I managed to fix the failing test, however I'm not sure if it's a good solution that I did.

Anyways, lemme know your thoughts, since I think I've addressed everything, except the optional -> required change.

[alexmccabe]

@bakerkretzmar I just merged the branch into here, any extra thoughts on this?

[alexmccabe]

@bakerkretzmar Howdy buddy, hope everything is okay. This has been sat for a while, I don't want to be annoying of course though

[bakerkretzmar]

@alexmccabe no worries, I've been travelling for the past several weeks! I'll take a more thorough look at this again when I'm home later this month! Appreciate it.

[alexmccabe]

@bakerkretzmar No worries, I hope you had a nice time travelling. Did you get a chance to look around this?

[bakerkretzmar]

@alexmccabe thanks a ton for your patience 🙏🏻 messed around with this some more and finally figured out some slightly simpler utility types that I'm happy with. The resulting type has an extra union which is a bit uglier, but I'm fine with that trade-off for the type definitions to be cleaner.

Playground for future reference.

Just going to double-check that the array argument thing was already an issue and didn't appear in this PR and then I'll get this merged!

[alexmccabe]

That's okay buddy, Open Source can take time. Thank you for considering the work!