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

[chore] expose Body generic to hook functions #2413

Merged
merged 5 commits into from
Sep 14, 2021
Merged

[chore] expose Body generic to hook functions #2413

merged 5 commits into from
Sep 14, 2021

Conversation

george-lim
Copy link
Contributor

@george-lim george-lim commented Sep 12, 2021

Closes #1215

PR #1256 allows us to avoid importing internal type ReadOnlyFormData for RequestHandler, since it has the Body generic. However, the hook functions don't expose the Body generic so we currently still need to import ReadOnlyFormData type if we want to process form data for post requests in our handle hooks.

This will enable users to pass in FormData for Body generic in GetSession, Handle, and HandleError.

image

@changeset-bot
Copy link

changeset-bot bot commented Sep 12, 2021

🦋 Changeset detected

Latest commit: ca081e9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Patch

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

@george-lim george-lim changed the title [fix] Fix hooks add body [fix] Add body generic to hook functions Sep 12, 2021
Copy link
Member

@ignatiusmb ignatiusmb left a 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> {
Copy link
Member

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.

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.

Copy link
Contributor Author

@george-lim george-lim Sep 13, 2021

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.

@ignatiusmb ignatiusmb changed the title [fix] Add body generic to hook functions [chore] expose Body generic to hook functions Sep 14, 2021
@ignatiusmb ignatiusmb merged commit d7189eb into sveltejs:master Sep 14, 2021
@ignatiusmb
Copy link
Member

thanks!

@george-lim george-lim deleted the fix-hooks-add-body branch September 14, 2021 15:24
sidharthv96 added a commit to sidharthv96/kit that referenced this pull request Sep 19, 2021
* '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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should ReadOnlyFormData type be exported?
4 participants