-
Notifications
You must be signed in to change notification settings - Fork 203
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 cors utils #322
feat: add cors utils #322
Conversation
Thanks for PR dear @NozomuIkuta. Can we please directly inline the logic in h3 package? This way we can directly test and release it. |
README.md
Outdated
@@ -157,6 +157,7 @@ H3 has a concept of composable utilities that accept `event` (from `eventHandler | |||
- `getSession(event, { password, name?, cookie?, seal?, crypto? })` | |||
- `updateSession(event, { password, name?, cookie?, seal?, crypto? }), update)` | |||
- `clearSession(event, { password, name?, cookie?, seal?, crypto? }))` | |||
- `handleCors(options)` (see [h3-cors](https://github.com/NozomuIkuta/h3-cors) for more detail) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me consider it in another time how we can organize README, because it needs lots of words to explain CORS features. Or, we might want to create a Docus website for h3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. If possible adding help in JSDocs would be the best. In the future we should generate README baased on JSDocs and add more. Link is good for me until then 👍🏼
Codecov Report
@@ Coverage Diff @@
## main #322 +/- ##
==========================================
+ Coverage 71.92% 74.32% +2.39%
==========================================
Files 23 26 +3
Lines 2066 2294 +228
Branches 303 354 +51
==========================================
+ Hits 1486 1705 +219
- Misses 580 589 +9
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All nice work 💯 adding prefixes i think we can land it!
src/utils/cors/utils.ts
Outdated
return defu(options, defaultOptions); | ||
} | ||
|
||
export function isPreflight(event: H3Event): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe isPreflightRequest
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed: 4636416
src/utils/cors/types.ts
Outdated
} | ||
| EmptyHeader; | ||
|
||
export type AccessControlMaxAgeHeader = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are exposing this types to end user, prefix would be better. (Do we really need to expose all individuals?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed: 052c215
I modified exports and make only CorsOptions
public, which is useful to restrict argument type for CORS utilities (e.g. handleCors(event, corsOptions)
).
It's worth merging content of NozomuIkuta/h3-cors#8 into this PR. |
#322 (comment) is fixed: bb7af39. |
I applied your feedback and the remaining PR (NozomuIkuta/h3-cors#8). |
src/utils/cors/handler.ts
Outdated
preflight: { statusCode }, | ||
} = resolveCorsOptions(options); | ||
|
||
return defineEventHandler((event) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
handle shouldn't create a wrapper handler but directly accept event
to append required headers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed: c1eef73
I have pushed couple of refactors. Really nice stuff thanks for helping to bring it to the core 👍🏼 |
Resolves #82
Related: NozomuIkuta/h3-cors#10
This PR is created according to @pi0's comment to my package.
I have upgraded h3 version in my package (and improved code coverage to 100% 👍).
Now, please review this PR and check whether I'm doing as you intended.