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

Improve RequestHandler and EndpointOutput type declarations #1778

Merged

Conversation

dannyvelas
Copy link
Contributor

@dannyvelas dannyvelas commented Jun 29, 2021

If I wanted my post function to return a number as a response body and I accidentally return a string, there is no way I can let the type-checker prevent situations like these. There is no way I can tell it "hey, make sure the body property is always a number". So, the following typechecks:

export const post: RequestHandler<Locals, MyBody> = async (request) => {
  return {
    headers: { "set-cookie": "jwt=token; Path=/; HttpOnly" },
    body: "wrong type"
  };
};

This PR changes RequestHandler and EndpointOutput so that users can specify their response body type as number by making number the third generic argument to RequestHandler. As a result, this won't typecheck:

export const post: RequestHandler<Locals, MyReqBody, number> = async (request) => {
  return {
    headers: { "set-cookie": "jwt=token; Path=/; HttpOnly" },
    body: "wrong type",
  };
};

I wanted to make the second generic parameter to RequestHander, ReqBody be of default type DefaultBody, but this causes an error in packages/kit/src/runtime/server/endpoint.js on line 27, because the request.body: BaseBody is not type compatible with BaseBody & DefaultBody, which is the type it would end up with (per the type definitions of ServerRequest and ParametrizedBody).

There might be a better way to avoid the error and give ReqBody stronger typing than unknown, but I don't currently know enough about the codebase to do so.

Background for this PR: In the discord channel contribute-to-svelte-kit, I proposed this idea a few messages from the last.

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts

@changeset-bot
Copy link

changeset-bot bot commented Jun 29, 2021

🦋 Changeset detected

Latest commit: 27d8b34

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

…t or Response bodies. Added a generic parameter to `RequestHandler` and an generic parameter to `EndpointOutput`. Together, these let you specify the type of the body of the Response you want to return from a RequestHandler function. The default of both of these parameters is `DefaultBody`.
@dannyvelas dannyvelas force-pushed the request-handler-type-improvement branch from 76d75eb to b1d5f97 Compare June 29, 2021 18:04
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.

This looks good, just a couple of comments. A changeset would also be nice.

packages/kit/types/endpoint.d.ts Outdated Show resolved Hide resolved
packages/kit/types/endpoint.d.ts Outdated Show resolved Hide resolved
packages/kit/types/endpoint.d.ts Outdated Show resolved Hide resolved
…and `RequestHandler`. Also added patch changeset
@dannyvelas
Copy link
Contributor Author

Cool. I just made the changes:

  1. Adding extends DefaultBody to the generic parameter of EndpointOutput and the last generic parameter of RequestHandler.
  2. Changed generic type names ReqBody and ResBody to Input and Output, respectively, in RequestHandler.
  3. Changed generic type name ResBody to Body in EndpointOutput.
  4. Added patch changeset, tall-hotels-punch.

Let me know if I misunderstood any of your suggestions - I'd be happy to go back and amend any mistakes.

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.

Thanks! Everything else looks good to me.

@dummdidumm dummdidumm merged commit 325c223 into sveltejs:master Jul 1, 2021
dummdidumm pushed a commit that referenced this pull request Jul 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants