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

Widen type defs to allow for nonstring version constraints #183

Closed

Conversation

danielmoore
Copy link

find-my-way's design makes it agnostic as to the specific type and shape of a version, however the bundled types constrain versions to a string. This PR does three things:

  1. Provides a test showing that non-string versions are supported.
  2. Widens the typings to a generic, so that implemented strategies are consistently typed, with a string version as a default.
  3. Marks validate as optional, because it is.

@danielmoore danielmoore force-pushed the nonstring-custom-constraint branch 2 times, most recently from cf68a71 to d291da2 Compare March 13, 2021 20:27
@mcollina
Copy link
Collaborator

can you add a test for the type in test/types/router.test-d.ts?

@mcollina
Copy link
Collaborator

good work!

@danielmoore
Copy link
Author

Thanks! I've added some type tests. I'm new to type-testing, so any feedback you have on it would be great.

Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@@ -57,17 +57,17 @@ declare namespace Router {
store: any
) => void;

interface ConstraintStrategy<V extends HTTPVersion> {
interface ConstraintStrategy<V extends HTTPVersion, ConstraintVersion = string> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might make more sense to call this property ConstraintValue or something like that. I know it says version in all the types below, but these strategies can actually be used for other constraints than version, like host or maybe the Accept header someday. I think the name of that argument in the types should be changed too.

@@ -91,7 +91,7 @@ declare namespace Router {
): void;

constraints? : {
[key: string]: ConstraintStrategy<V>
[key: string]: ConstraintStrategy<V, unknown>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like it might be a breaking change, do the fastify type tests still pass with this in place?

@ivan-tymoshenko
Copy link
Collaborator

I guess it was implemented here #180. We can close it.

@mcollina mcollina closed this May 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants