-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
[chore] expose Body generic to hook functions #2413
Conversation
🦋 Changeset detectedLatest commit: ca081e9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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.
Thank you, can we also update them in the docs
@@ -16,19 +16,19 @@ export interface ServerResponse { | |||
body?: StrictBody; | |||
} | |||
|
|||
export interface GetSession<Locals = Record<string, any>, Session = any> { | |||
(request: ServerRequest<Locals>): MaybePromise<Session>; | |||
export interface GetSession<Locals = Record<string, any>, Body = unknown, Session = any> { |
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.
Inserting the argument in the middle will be a breaking change, but it does make sense considering the usage order and keeps it consistent with the order from other types like Load
. Let's make sure to also update the docs accordingly so users can crosscheck properly.
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.
Alternatively, one could use variadic tuples if BC should be kept: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-0.html
This would result in more work and bloat in the implementation, so I would personally vote against it. Just wanted to point out that it would be possible.
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.
@ignatiusmb I updated the docs to reflect the changes.
@benbender I agree with you there, it's cool that it's possible but I don't think it's worth the bloat.
thanks! |
* 'master' of https://github.com/sveltejs/kit: (322 commits) Version Packages (next) (sveltejs#2438) [fix] revert sveltejs#2354 and fix double character decoding a different way (sveltejs#2435) chore: update dependencies (sveltejs#2447) [docs] add link to envPrefix option in env var FAQ (sveltejs#2445) Fix invalid changeset. Thanks GitHub bot :-p [feat] use the Vite server options in dev mode (sveltejs#2232) [fix] provide valid value for `letter-spacing` CSS property in template (sveltejs#2437) [docs] fix typo (sveltejs#2443) [fix] add svelte field when packaging (sveltejs#2431) Version Packages (next) (sveltejs#2428) [chore] update lockfile [fix] update to TS 4.4.3 (sveltejs#2432) [chore] add links to repository and homepage to package.json (sveltejs#2425) docs: use fragment for prefetching link (sveltejs#2429) [fix] encodeURI during prerender (sveltejs#2427) Version Packages (next) (sveltejs#2420) revert change to versioning during release workflow chore: update vite-plugin-svelte (sveltejs#2423) [chore] expose Body generic to hook functions (sveltejs#2413) [feat] adapter-node entryPoint option (sveltejs#2414) ...
Closes #1215
PR #1256 allows us to avoid importing internal type
ReadOnlyFormData
for RequestHandler, since it has theBody
generic. However, the hook functions don't expose theBody
generic so we currently still need to importReadOnlyFormData
type if we want to process form data for post requests in our handle hooks.This will enable users to pass in
FormData
forBody
generic in GetSession, Handle, and HandleError.