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

feat: add event handler generics for typed request body and query #417

Merged
merged 19 commits into from
Jul 27, 2023

Conversation

danielroe
Copy link
Member

resolves #129

this adds support explicit typing of input for event handlers, which will unlock autocomplete when consumed in $fetch down the line (in nitro).

I've kept the default return types of any intact for backwards compatibility, and also added backwards compatibility for existing generic order, but I would advise reconsidering this (see #387).

@danielroe danielroe added the enhancement New feature or request label Jun 29, 2023
@danielroe danielroe requested a review from pi0 June 29, 2023 14:07
@danielroe danielroe self-assigned this Jun 29, 2023
@codecov
Copy link

codecov bot commented Jun 29, 2023

Codecov Report

Merging #417 (3a9fd8b) into main (29445c4) will increase coverage by 0.36%.
The diff coverage is 98.75%.

@@            Coverage Diff             @@
##             main     #417      +/-   ##
==========================================
+ Coverage   79.24%   79.61%   +0.36%     
==========================================
  Files          27       27              
  Lines        2924     2977      +53     
  Branches      437      437              
==========================================
+ Hits         2317     2370      +53     
  Misses        607      607              
Files Changed Coverage Δ
src/utils/body.ts 95.43% <92.85%> (+0.07%) ⬆️
src/event/event.ts 75.67% <100.00%> (+0.44%) ⬆️
src/event/utils.ts 72.38% <100.00%> (+8.18%) ⬆️
src/types.ts 100.00% <100.00%> (ø)
src/utils/request.ts 98.60% <100.00%> (+0.05%) ⬆️

@pi0
Copy link
Member

pi0 commented Jun 30, 2023

We have quickly talked with @danielroe about this.

It is a really nice idea to support typed input (body, query, headers, etc) and output for the event handler interface.

Only pending this future to first introduce a simple version of object syntax to see if there are any alternative ideas for defining generics (such as infering io types from middleware) and avoiding breaking changes.

@Hebilicious Hebilicious added the pending label Jun 30, 2023 — with Volta.net
Copy link
Member Author

I don't see why this change shouldn't be compatible with future object syntax. The generic would simply point to a different property of the object. (Nothing about how this is written should forestall inferring type.)

Having said that, I'm happy to defer this, but would love to see forward movement on typed input if possible.

Copy link
Member

@Hebilicious Hebilicious left a comment

Choose a reason for hiding this comment

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

Could you precise how end users will use this ? Unlike validation, this type can't really be trusted. What does this unblock for $fetch / useFetch exactly ?

src/utils/body.ts Outdated Show resolved Hide resolved
@danielroe
Copy link
Member Author

Could you precise how end users will use this ? Unlike validation, this type can't really be trusted. What does this unblock for $fetch / useFetch exactly ?

This allows us to granularly specify upstream and downstream types.

So $fetch could specifically type an event handler with typed inputs, and disable calling $fetch('/some/endpoint') without specifying the correct body and/or query string.

This PR also implements downstream types for event, for example:

export default defineEventHandler<{ body: CustomBody }>(event => {
  const body = await readBody(event)
  // body is typed
})

(However, the main point is to allow upstream typing of calling $fetch)

You are right that this is unsafe but it is currently typed as any, which is much more unsafe. Ideally we would support validators in the new object syntax, which is fully compatible with this PR, which would provide the body/query input types without the user specifying them in the generic, and additionally provide runtime safety.

As I've said, the fact that users can specify types for body without validating them is still better than the current implementation - and additionally provides the safety of enforcing types via internal calls via $fetch.

@Hebilicious
Copy link
Member

Hebilicious commented Jul 10, 2023

Could you precise how end users will use this ? Unlike validation, this type can't really be trusted. What does this unblock for $fetch / useFetch exactly ?

This allows us to granularly specify upstream and downstream types.

So $fetch could specifically type an event handler with typed inputs, and disable calling $fetch('/some/endpoint') without specifying the correct body and/or query string.

This PR also implements downstream types for event, for example:

export default defineEventHandler<{ body: CustomBody }>(event => {
  const body = await readBody(event)
  // body is typed
})

(However, the main point is to allow upstream typing of calling $fetch)

You are right that this is unsafe but it is currently typed as any, which is much more unsafe. Ideally we would support validators in the new object syntax, which is fully compatible with this PR, which would provide the body/query input types without the user specifying them in the generic, and additionally provide runtime safety.

As I've said, the fact that users can specify types for body without validating them is still better than the current implementation - and additionally provides the safety of enforcing types via internal calls via $fetch.

Thanks for the explanation ! For reference I'll tag #431 for runtime validation support

@pi0
Copy link
Member

pi0 commented Jul 26, 2023

@danielroe Can you please verify my latest changes? 🙏🏼

@pi0 pi0 changed the title feat: add event handler generic for typed input body/query feat: add event handler generics for typed input body and query Jul 26, 2023
@pi0 pi0 changed the title feat: add event handler generics for typed input body and query feat: add event handler generics for typed request body and query Jul 26, 2023
@pi0 pi0 merged commit fd687b3 into main Jul 27, 2023
6 checks passed
@pi0 pi0 deleted the feat/input-types branch July 27, 2023 15:40
@danielroe danielroe mentioned this pull request Aug 3, 2023
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request pending
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support additional generic for typing incoming body & query
3 participants