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: generate InternalFetch signature #1532

Closed
wants to merge 25 commits into from
Closed

Conversation

danielroe
Copy link
Member

@danielroe danielroe commented Aug 4, 2023

πŸ”— Linked issue

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

This PR reworks how our internal API routes are typed. This aims to do more of the work at build time rather than asking typescript to do it. Testing in our fixture showed type-checking speed improvement of up of 45%.

Note: The reason that this is so much faster is that we use function overloads, which means that we do not yet support all the matching patterns that our existing type-only algorithm does. It matches only one result so if you have multiple overlapping patterns, like /api/test/**/thing and /api/test/bob/thing then if you request $fetch('/api/test/${someUnknownString}') then we won't be able to give a union of the two return times as before.

It adds the following type-safety functionalities:

  • it will throw a type error if you make a request that needs a specific method without an options object with that method defined
  • it will throw a type error if you make a request that needs specific input types without those values
  • type error if you make a request that needs specific input types without an options object
  • when $fetch.create is called with a baseURL, the result is not strongly typed but just a normal external $fetch

Further enhancements needed:

  • auto-completion when used with endpoints with typed inputs (this would be very desirable for better DX)

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@danielroe danielroe added the enhancement New feature or request label Aug 4, 2023
@danielroe danielroe self-assigned this Aug 4, 2023
src/build.ts Show resolved Hide resolved
src/build.ts Outdated Show resolved Hide resolved
@pi0 pi0 mentioned this pull request Aug 6, 2023
7 tasks
@codecov
Copy link

codecov bot commented Aug 9, 2023

Codecov Report

Merging #1532 (7290307) into main (7fa8408) will increase coverage by 0.27%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

❗ Current head 7290307 differs from pull request most recent head 2cc871f. Consider uploading reports for the commit 2cc871f to get more accurate results

@@            Coverage Diff             @@
##             main    #1532      +/-   ##
==========================================
+ Coverage   76.49%   76.76%   +0.27%     
==========================================
  Files          73       73              
  Lines        7580     7671      +91     
  Branches      751      768      +17     
==========================================
+ Hits         5798     5889      +91     
  Misses       1781     1781              
  Partials        1        1              
Files Changed Coverage Ξ”
src/build.ts 87.58% <100.00%> (+1.64%) ⬆️
src/types/fetch.ts 100.00% <100.00%> (ΓΈ)

@danielroe danielroe marked this pull request as ready for review August 9, 2023 08:41
@danielroe danielroe requested a review from pi0 August 9, 2023 08:49
@danielroe danielroe marked this pull request as draft August 9, 2023 11:28
@manniL
Copy link
Contributor

manniL commented Aug 18, 2023

Note: The reason that this is so much faster is that we use function overloads, which means that we do not yet support all the matching patterns that our existing type-only algorithm does. It matches only one result so if you have multiple overlapping patterns, like /api/test/**/thing and /api/test/bob/thing then if you request $fetch('/api/test/${someUnknownString}') then we won't be able to give a union of the two return times as before.

Will this be possible in the future with the function overloading approach?

@septatrix
Copy link
Contributor

Will it be possible with the way it is implemented in this PR to augment the OpenAPI documentation #1162?

@danielroe danielroe mentioned this pull request Oct 11, 2023
8 tasks
@ImBoop
Copy link

ImBoop commented Aug 2, 2024

This PR has been outstanding as a draft for a year now, are there any updates you can share?

@pi0 pi0 added the types label Sep 20, 2024
@pi0 pi0 mentioned this pull request Sep 28, 2024
@pi0
Copy link
Member

pi0 commented Sep 28, 2024

This is taking longer than expected because we are working on a better standard ecosystem-wise and testable solution.

I have created a tracker issue: #2758

(closing this one mainly for triage, let's keep track via issue)

@pi0 pi0 closed this Sep 28, 2024
@danielroe danielroe deleted the feat/typed-inputs branch September 28, 2024 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request types
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants