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

feat: generated types for page and endpoint paths #9938

Closed
wants to merge 2 commits into from

Conversation

difanta
Copy link

@difanta difanta commented May 16, 2023

Please don't delete this checklist! 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 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 script to generate paths and types for endpoints, so that fetch calls can return a typed response both with fetch in a load function and global.fetch.
A .svelte-kit/types/$api.d.ts file is generated during sync that contains path and type informations.
I also extended this to ensure that fetch, goto and redirect point to a correct path, avoiding misspelling or errors when refactoring a site.
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 from discussions:

I've created a GitHub Project to share the tasks and information on this experiment https://github.com/users/difanta/projects/1.
I think enough work has been made to prove that this functionality works and could be a valid addition to Kit, now a discussion must be made to see if this is actually a thing that we want to merge or it's not exactly what we want. If this is accepted i will happily finish the remaining tasks.
I understand that tRPC seems like the obvious and more powerful solution here, but this is a zero-code addition that i think would benefit a lot of users, tRPC could still be added by those who feel like this is not enough.

New Features:

  • Fetch is only allowed with external urls, paths that exist as an endpoint (+server.js/ts)
  • When Fetching from an endpoint the response.json() method is typed with the returned type from that endpoint
  • Fetch is only allowed with the correct method (or no method specified if GET is valid in that endpoint)
  • Goto is only allowed with paths that exist as a route, or with any URL object
  • Redirect is only allowed with external urls or with paths that exist as a route or endpoint
  • auto completion on the allowed methods when writing the second argument of a Fetch call
  • global Fetch function returns a typed response if the path is correct, but does not throw errors when the path is not correct, i had trouble to eliminate the behavior of falling back to the default Fetch function when all the new overloads throw an error (of mistaken path)

Path matching:
to get all these functions to actually check the path passed to them an argument with type different from string must be passed, for example:

  • Fetch("/api/foo") is good because the argument type is "/api/foo"
  • Fetch("/api" + "/foo") or let path = '/api/foo'; Fetch(path) are not good as the argument type is string, these are allowed but will not be checked for misspelling and will not have type on the response
  • If a parameter is present in the path it must be passed with a string template, Fetch(`/api/foo/${params.bar}`). It works for any parameter combination (required, optional, rest)
  • If you want a Fetch to be allowed in any case you can pass Fetch("/api/..." as string) so that the function cannot infer the path
  • Fetches using URLs are allowed and unchecked/untyped
  • Having set a base path on the app works as expected, during the 'sync' command the base path is prepended to all paths and is also injected into an interface that modifies import { base } from "$app/paths"; to match exactly that path (no longer a unspecified '' | `${string}`),
    so Fetch(`${base}/api/foo`) works both if the path was or wasn't set
  • Fetching from Asset path is not supported still, more on that on the project
  • Goto path matching
  • Redirect path matching
  • Fetch path matching

Fetch types:
Currently fetch types work with a specific endpoint syntax, with the export const GET = async function () { ... } satisfies RequestHandler constraint and not the default export const GET: RequestHandler = () { ... } where it seems impossible to know the return type of the path.
This should be easily fixed with a proxy rewrite of +server.js/ts files like it is done with load functions, just a heads up.
The type returned from a Fetch call is TypedResponse<T> | TypedResponse<App.Error>, and checking for response.ok discriminates between the two. This was my choice and i think it promotes good code, although i do not know if all 'not ok' response actually return an App.Error object.
Fetch typed response

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) {
  // data: T
  const data = await response.json(); 
} else {
  // error: App.Error
  const error = await response.json(); 
}

@changeset-bot
Copy link

changeset-bot bot commented May 16, 2023

🦋 Changeset detected

Latest commit: 3b79bcc

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

@difanta
Copy link
Author

difanta commented May 25, 2023

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

@ryoppippi
Copy link

Any updates on this?

@difanta
Copy link
Author

difanta commented Jul 17, 2023

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
Copy link
Author

difanta commented Jul 27, 2023

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 difanta marked this pull request as ready for review July 27, 2023 16:36
@difanta
Copy link
Author

difanta commented Jul 27, 2023

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.

@nounderline

This comment was marked as outdated.

@bartaldavid
Copy link

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.

@eltigerchino eltigerchino marked this pull request as draft November 11, 2023 05:52
@eltigerchino eltigerchino changed the title Typed responses when fetching from endpoints feat: generated types for page and endpoint paths Nov 11, 2023
@eltigerchino
Copy link
Member

eltigerchino commented Nov 11, 2023

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?

@hmnd
Copy link
Contributor

hmnd commented Nov 11, 2023

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.

@difanta
Copy link
Author

difanta commented Nov 11, 2023

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

@AlbertMarashi
Copy link

AlbertMarashi commented Nov 23, 2023

@difanta What do you think about the approach I described in #647 (comment) ?

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
Copy link

AlbertMarashi commented Nov 27, 2023

@difanta can you check out the approach I described in #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()

@difanta
Copy link
Author

difanta commented Nov 27, 2023

@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

@AlbertMarashi
Copy link

AlbertMarashi commented Nov 28, 2023

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

…tch type safety when fetching from endpoints
@difanta
Copy link
Author

difanta commented Apr 3, 2024

Closing this in favour of #11108

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants