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

fix(types): enhance reusability of pres #4470

Merged
merged 1 commit into from
Mar 13, 2024
Merged

Conversation

IJuanI
Copy link
Contributor

@IJuanI IJuanI commented Oct 28, 2023

Support for pre handlers whose type definition is only a subset of the actual route type definition.

When trying to reuse pre handlers in a typescript environment like the following

const doSomething: Lifecycle.Method<any> = async ({ params }) => {
  console.log(params.preParam);
};

server.route<{ Params: { param: string, preParam: number } }>({
    method: 'GET',
    path: '/route',
    handler: async ({ params }) => console.log(params.param),
    options: {
        pre: [ doSomething ]
    }
});

server.route<{ Params: { otherParam: string, preParam: number } }>({
    method: 'GET',
    path: '/route',
    handler: async ({ params }) => console.log(params.otherParam),
    options: {
        pre: [ doSomething ]
    }
});

The type definition of the pre handler does not have any possible valid value, hence needing to drop type checking using the any keyword in order for typescript not to throw an error.

This happens because the definition of Lifecycle.Method expects the request type to equal to the type provided in the Method generic, but in this scenario there're 2 distinct request types.

Note: Using unions of both request types is also invalid, I would need to dig deeper into why, but seems related to the usege of the keyof operator in another type definitions.

The changes in this PR update the Method type definition to require the request type to extend the type provided in the generic instead of checking for it to be equal.

This change allows to resolve the above scenario with type definitions like the following:

const doSomething: Lifecycle.Method<{ Params: { preParam: number } }> = async ({ params }) => {
  console.log(params.preParam);
};

As all route definitions contain a preParam: number param, they're successfully extending the Method's generic, passing type checking

@kanongil kanongil added the types TypeScript type definitions label Nov 6, 2023
@Marsup Marsup added this to the 21.3.4 milestone Mar 13, 2024
Support for pre handlers whose type ref is only a subset of the actual route type ref
@Marsup
Copy link
Contributor

Marsup commented Mar 13, 2024

Thanks! Sorry this went under the radar for so long, I'm releasing it now.

@Marsup Marsup merged commit 2766ed2 into hapijs:master Mar 13, 2024
12 of 15 checks passed
@Marsup
Copy link
Contributor

Marsup commented Mar 13, 2024

Sorry, I had to revert it. It's breaking the final types of the request.

@IJuanI
Copy link
Contributor Author

IJuanI commented Sep 20, 2024

Hey @Marsup, thanks for taking a look!

This patch has been running in my company for almost a year with no typing issues. Any chance you could either re-run the failing CI, share the error you found, or provide testing instructions?

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
types TypeScript type definitions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants