kit icon indicating copy to clipboard operation
kit copied to clipboard

Typed responses when fetching from endpoints

Open difanta opened this issue 1 year ago • 2 comments

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • [x] 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
  • [x] This message body should clearly illustrate what problems it solves.
  • [ ] Ideally, include a test that fails without this PR but passes with it.

Tests

  • [x] Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • [x] If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Description

I made a typed endpoint generator for getting correct types when making a fetch call, both with load function and global.fetch. A .svelte-kit/types/$api.d.ts file is generated during sync and is imported in every $types.d.ts file. This seems like a graceful addition to svelte kit to get end-to-end typing and i personally really enjoyed having this option in my projects. This originated in part from discussions:

  • https://github.com/sveltejs/kit/issues/7110
  • https://github.com/sveltejs/kit/issues/647
  • https://github.com/sveltejs/kit/issues/9732

Fetch gains a special type definition when the path is a string that matches a existing path with a +server.js/ts endpoint. The return type of response.json() matches that of the endpoint when the latter returns using json() function. Autocompletion works when writing a fetch call with a matching path and provides the possible methods (GET, POST, ecc...) that can be called based on the +server.js/ts exported methods. It is allowed to fetch from an endpoint without specifying the method only if the endpoint has a GET handler, a type error is shown otherwise, and a type error is also shown when fetching with a non existing method.

If a fetch contains a static string like "/api/path/foo" all works well, including possible route parameters (required, optional or rest). For paths that contain dynamic parameters it is required to write it as a string template like this: `/api/path/${param}/${other_param}` and these parameters are treated as valid parameter strings no matter what, because it is impossible to know what their value will be at runtime. If there are more than one matching endpoint for a given path (because of optional parameters and such) a union of possible types is returned.

A final remark is about return type extraction from Endpoints, if using the syntax export const GET: RequestHandler = ... it seems impossible to know the return type of the path, while using export const GET = async function () { ... } satisfies RequestHandler allows types to be inferred. I think i need to implement proxy file generation as it is done with load functions, altough i don't know much about it yet.

Update

I recently updated the return type of response.json() to return TypedResponse<T> | TypedResponse<App.Error>. Checking for response.ok yields either TypedResponse<T> or TypedResponse<App.Error> after the check, this is because a fetch could either go well and return the correct response, or fail for some reason and it should always return an error in the form of App.Error. This type tries to mimick that behaviour. This led to some changes to existing code and SvelteKit tests, because not all fetch calls then check for response.ok before getting the type through response.json(). This may be a breaking change.

const response = await fetch(
    `/api/Campaign/${params.campaign}/dashboards/${params.dashboard}`
  );
const unchecked_data = await response.json();
// unchecked_data: App.Error | T
if (response.ok) {
  const data = await response.json(); 
  // data: T
} else {
  const error = await response.json(); 
  // error: App.Error
}

In the last update i also added a way to make this work when calling fetch from global.fetch and not from a load function as a parameter. Putting my fetch type into declare global { ... } allows the type definition to be correct without making imports but has no autocompletion or type errors described above, because the standard fetch still exists in the global namespace and 'absorbs' all calls that would otherwise be of error type with my fetch.

Conclusions

This is not the most rigorous solution but it is probably the one that requires less code changes to make it work (almost none in fact), and is a non intrusive solution where it is not needed, because no code needs to change if types are not a requirement (all fetch types default to any if type can't be inferred or not returning via json()). I think it works really well for all use cases, the only exception being for apps with a lot of handleFetch handling in endpoints routes or many fetch calls that get redirected.

Other than a couple of problems this is currently working and i'm using it in my projects, with the only code change being the satisfies RequestHandler constraint on exported endpoint methods. Look forward to someone more experienced to take a look at this and give his opinion, cheers.

Global kit types were modified:

  • added FetchType to a few interfaces (defaulting to typeof fetch)
  • TypedResponse<T> extends Reponse added
  • json(data: any, init?) => Response became json<T>(data: T, init?) => TypedResponse<T>

difanta avatar May 16 '23 16:05 difanta

🦋 Changeset detected

Latest commit: 3b79bccde40e25e6dbd9374873cbdc698ad9e013

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 Minor

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

changeset-bot[bot] avatar May 16 '23 16:05 changeset-bot[bot]

Update: rewritten the PR and made an update on the way the response types should be read.

difanta avatar May 25 '23 00:05 difanta

Any updates on this?

ryoppippi avatar Jul 08 '23 16:07 ryoppippi

In these past weeks i didn't have any spare time to continue this, however i plan to continue in the summer. I plan to explore the possibility of extending this to some other issues that have come up, like "path safety" on redirect() and goto(), where you get a type error when misspelling a path. Will update as soon i get anything done.

difanta avatar Jul 17 '23 14:07 difanta

It took not long to complete what i said earlier, now i would like to see if the main contributors like this proposal, and to discuss it. The PR has been rewritten to accurately represent the new features (the changeset file is not representative yet, sorry!)

difanta avatar Jul 27 '23 16:07 difanta

i should add that this is not ready to be merged, but rather to decide whether to merge it in the future once completed or to leave it be.

difanta avatar Jul 27 '23 16:07 difanta

Any plan to get it merged? It's done @dummdidumm

nounderline avatar Sep 15 '23 21:09 nounderline

Any updates on this? Nuxt has this feature and makes api endpoints a joy to use. In sveltekit currently I can either use hacky methods to extend the Response type and a map between routes and responses, or use trpc, which is an unneccessary overhead for just managing types between backend and frontend.

bartaldavid avatar Nov 10 '23 17:11 bartaldavid

Just a heads up: the merge conflict comes from throw redirect() now being able to accept a URL object in addition to a string. I'm not sure we can really add type safety for those URL objects? And I think that might be okay?

eltigerchino avatar Nov 11 '23 06:11 eltigerchino

Just a heads up: the merge conflict comes from throw redirect() now being able to accept a URL object in addition to a string. I'm not sure we can really add type safety for those URL objects? And I think that might be okay?

I agree. I wouldn't expect type safety with a URL object.

hmnd avatar Nov 11 '23 22:11 hmnd

Yep URL objects are not typed. Since redirect was changed I did not edit the code but it would be a small fix to at least accept URL objects much like 'goto'. I'm honestly just waiting to see some feedback from the main contributors before putting in any more work into this PR

difanta avatar Nov 11 '23 22:11 difanta

@difanta What do you think about the approach I described in https://github.com/sveltejs/kit/issues/647#issuecomment-1823759148 ?

We get typed URLs, along with typed responses and also ability to later extend it to support a typed request body, and removes the complexities with trying to type URL params & rest params

I am considering creating a new PR that implements this

AlbertMarashi avatar Nov 23 '23 04:11 AlbertMarashi

@difanta can you check out the approach I described in https://github.com/sveltejs/kit/pull/11108, maybe we can collaborate on a proposal.

I think there's a few issues with approaching the URL strings using template syntax (eg: /api/Campaign/${params.campaign}/dashboards/${params.dashboard}) - particularly around clarity and errors and how clear [..rest] parameters work.

I think it makes a lot of sense to have a fetch wrapper that takes in the RouteId and maps it to a certain response type with operator overloading

eg: /api/posts/[post]/$types.d.ts

....

namespace "$api" {
    export async function api_fetch(url: "/api/posts/[post]", options?: FetchOptions<ApiPostsPostParamParams, "GET">): Promise<TypedResponse<{ foo: string }>>;
}

We'd then call this and provide the parameters for the URL via the options, eg:

import { api_fetch } from "$api"

let res = await api_fetch("/api/posts/[post]", {
    method: "GET",
    params: {
        post: 123
    })
    
let json = await res.json()

AlbertMarashi avatar Nov 27 '23 22:11 AlbertMarashi

@AlbertMarashi Yeah yesterday I read your pr and comments and I like them a lot, I am trying a few things then I'll write my opinion, overall I think your wrapper could work better

difanta avatar Nov 27 '23 22:11 difanta

Great, looking forward to hearing feedback and collaborating. It would be really nice if TypeScript had https://github.com/Microsoft/TypeScript/issues/6579 supported but unfortunately doing the params in a templated string presents a lot of difficulties from my experimentation

AlbertMarashi avatar Nov 28 '23 22:11 AlbertMarashi

Closing this in favour of #11108

difanta avatar Apr 03 '24 10:04 difanta