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: request validation using handler object syntax validate and validateEvent(event) #496

Draft
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

danielroe
Copy link
Member

πŸ”— Linked issue

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

This allows inferring type of event by passing a validate function which needs to return the event (as validated). It's simple and allows any kind of validation, throwing an error if invalid input is provided.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@danielroe danielroe added the enhancement New feature or request label Aug 7, 2023
@danielroe danielroe requested a review from pi0 August 7, 2023 12:02
@danielroe danielroe self-assigned this Aug 7, 2023
@codecov
Copy link

codecov bot commented Aug 7, 2023

Codecov Report

Attention: Patch coverage is 95.90164% with 5 lines in your changes missing coverage. Please review.

Project coverage is 83.19%. Comparing base (a58d7c9) to head (6f2ea16).
Report is 85 commits behind head on main.

❗ Current head 6f2ea16 differs from pull request most recent head 58c0256

Please upload reports for the commit 58c0256 to get more accurate results.

Files Patch % Lines
src/event/utils.ts 97.01% 2 Missing ⚠️
src/utils/request.ts 50.00% 2 Missing ⚠️
src/event/event.ts 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #496      +/-   ##
==========================================
+ Coverage   77.83%   83.19%   +5.36%     
==========================================
  Files          47       32      -15     
  Lines        4286     3654     -632     
  Branches      611      545      -66     
==========================================
- Hits         3336     3040     -296     
+ Misses        933      614     -319     
+ Partials       17        0      -17     

β˜” View full report in Codecov by Sentry.
πŸ“’ Have feedback on the report? Share it here.

@pi0
Copy link
Member

pi0 commented Aug 7, 2023

I am wondering if we can alternatively support before:[validate()] to allow multiple validate functions and avoid adding new hook.

@danielroe
Copy link
Member Author

The issue is the typing. It might be possible but will be a lot more complex, to create chainable validators. (So output of one becomes input of another.)

@pi0
Copy link
Member

pi0 commented Aug 8, 2023

It makes sense for type complexity

I am thinking it could give some overhead for when we want to do body normalization, we actually read the body (and an external library can apply schema normalization). Only returning a boolean + typed Event will cause to loose this information (unless the utils support a context caching mechanism for when we later call useValidatedBody AND we use same validation options) vs before hooks or alike method that can do both validation + context injection like before: [useValidatedBody]

Also, it would be nice if we expose a generic utility async validateEvent to extract this functionality and also make it available for non object syntax usages.

@pi0 pi0 changed the title feat: add validate handler to object syntax feat: validate events using object syntax validate and validateEvent(event) Aug 8, 2023
@pi0 pi0 changed the title feat: validate events using object syntax validate and validateEvent(event) feat: request validation using handler object syntax validate and validateEvent(event) Aug 8, 2023
@iainsproat
Copy link
Contributor

Hi, I wish to understand how this change would interact with, or eliminates the need for, the validation options via community packages mentioned in h3's README? https://github.com/unjs/h3#community-packages

src/event/utils.ts Outdated Show resolved Hide resolved
@pi0
Copy link
Member

pi0 commented Aug 14, 2023

Update: Both me and @danielroe need to rethink about whole typing and validation and how it works with downstream (Nitro and Nuxt) types and best type/runtime safely+perf balance which is tricky at the moment. Making it draft until after 1.8 to iterate.

@pi0 pi0 marked this pull request as draft August 14, 2023 10:54
@pi0 pi0 self-assigned this Aug 14, 2023
Copy link

cloudflare-workers-and-pages bot commented Jun 19, 2024

Deploying unjs-h3 with Β Cloudflare Pages Β Cloudflare Pages

Latest commit: 58c0256
Status:Β βœ…Β  Deploy successful!
Preview URL: https://1324ba9c.unjs-h3.pages.dev
Branch Preview URL: https://feat-validate.unjs-h3.pages.dev

View logs

@pi0 pi0 force-pushed the feat/validate branch from 5ff33bd to 58c0256 Compare June 19, 2024 12:12
@pi0 pi0 added the v2 label Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request v2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants