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: enable response type infer for API routes with params #222

Merged
merged 33 commits into from Aug 1, 2022

Conversation

didavid61202
Copy link
Contributor

@didavid61202 didavid61202 commented May 10, 2022

πŸ”— Linked issue

resolves nuxt/nuxt#10978, nuxt/nuxt#14187

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 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)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

Update MatchedRoutes helper type in types/fetch.ts to enable $fetch to infer response type even for routes with params, (ex: users/[useId]/posts/[postId].ts)

This will provide better DX for a wider range of complex string requests with variables in it for requesting server routes containing params

This would also further provide Nuxt 3's useFetch and useLazyFetch to be able to infer the response's data type as well. 😎

Usage scenario

assume the project contains following API file structure

nitroTestApiFileStructure

with each file export a default h3 eventHandler, for example:

// routes\users\[userId].ts
export default eventHandler(() => {
    return { username: 'David', age: 18 }
})

and use in other file as

declare const params
cosnt { userId, postId } = params

const user = await $fetch(`/api/user/${userId}`)
// user's type will be 
// { 
//    username: string; 
//    age: number; 
// }

const post = await $fetch(`/api/user/${userId}/post/${postId}`) // param match
// post's type will be the return type of API routes:
//    '/api/user/:userId/post/:postId'  |  '/api/user/:userId/post/firstPost' | 
//    '/api/user/john/post/:postId'  |   '/api/user/john/post/coffee'

// support glob matching
const johnPost= await $fetch(`/api/user/john/post/${postId}/**`) // exact match
// johnPost's type will be the return type of API routes: '/api/user/john/post/:postId'

// while if :userId is sure not matching 'john', will exclude all API with '/api/user/john/**'
const otherUserPost = await $fetch(`/api/user/someUserId/post/${postId}`)
// otherUserPost's type will be the return type of API routes: 
//    '/api/user/:userId/post/:postId'   |   '/api/user/:userId/post/firstPost'

const coffeePost= await $fetch(`/api/user/john/post/coffee`) // exact match
// coffeePost's type will be the return type of API routes: '/api/user/john/post/coffee'

const firstPost = await $fetch(`/api/user/${userId}/post/firstPost`) // param and partial exact match
// firstPost's type will be the return type of API routes: '/api/user/:userId/post/firstPost'

const todos = await $fetch(`/api/todos/some/deeply/nest/${dynamicString}/path`) // match globs
// todos's type will be the return type of API routes: '/api/todos/**'

const comments = await $fetch(`/api/todos/${dynamicString}/comments/foo/bar/baz`) // match globs
// comments's type will be the return type of API routes: 
//    '/api/todos/**'  |  '/api/todos/:todoId/comments/**:commentId'

const result = await $fetch('/api/otherPath/unknown')
// result's type will be 'unknown'

πŸ“ Checklist

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

@didavid61202
Copy link
Contributor Author

didavid61202 commented May 10, 2022

Update

this is fixed with PR


btw, I've asked in Nuxt's Dicord/typescript room about playgound/tsconfig.json setup for correctly inferring nitro's InternalApi type during dev.

@danielroe
Copy link
Member

Would also be nice to support globs, such as /api/foo/**. Do you think you would be able to add that @didavid61202?

@didavid61202
Copy link
Contributor Author

Would also be nice to support globs, such as /api/foo/**. Do you think you would be able to add that @didavid61202?

on it! πŸ‘Œ

@didavid61202
Copy link
Contributor Author

@danielroe Hi! πŸ‘‹
I've updated and refactored the MatchRoutes type to support globs user/id/**. And improved the matching result by using different priorities of exact, partial, exact + partial, template string/const string matches.

I also added some type tests using the expect-type package.

please check the updated PR description or the type tests code for details. πŸ™Œ

Thanks for your time and effort working on this and guiding me. ☺️

@danielroe
Copy link
Member

danielroe commented May 29, 2022

This is looking great - just want to clarify what I meant by globs.

If we have a key in InternalApi like this:

export interface InternalApi {
  '/api/user/:userId/post/**': SomeReturnType
}

Then this should match /api/user/234/post and also /api/user/2348/post/parent and /api/user/2348/post/parent/child.

@didavid61202
Copy link
Contributor Author

didavid61202 commented May 29, 2022

This is looking great - just want to clarify what I meant by globs.

If we have a key in InternalApi like this:

export interface InternalApi {
  '/api/user/:userId/post/**': SomeReturnType
}

Then this should match /api/user/234/post and also /api/user/2348/post/parent and /api/user/2348/post/parent/child.

Got it, I got it the wrong way around πŸ€¦β€β™‚οΈ πŸ˜‚
working on it πŸ‘Œ

@didavid61202
Copy link
Contributor Author

didavid61202 commented May 29, 2022

Implemented globs πŸŽ‰
please check if it is working as expected when you are available, Thanks ☺️

I think this test case @L84 should also include Api key: /api/todos/:todoId/comments/**:commentId, @danielroe what is your opinion ? πŸ€”

Copy link
Member

@danielroe danielroe 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 to me. ✨

Side note: I'm cautious about potential type complexity in nitro but I'd be happy to merge and refactor type implementation down the line if needed. (For example, perhaps we could explicitly generate /api/${string} rather than /api/:slug as the key of InternalApi if we find the current approach has negative perf implications.)

@codecov
Copy link

codecov bot commented Aug 1, 2022

Codecov Report

Merging #222 (3622bb6) into main (7aaab6d) will increase coverage by 55.94%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##           main     nuxt/framework#222       +/-   ##
=========================================
+ Coverage      0   55.94%   +55.94%     
=========================================
  Files         0       54       +54     
  Lines         0     3466     +3466     
  Branches      0      362      +362     
=========================================
+ Hits          0     1939     +1939     
- Misses        0     1178     +1178     
- Partials      0      349      +349     
Impacted Files Coverage Ξ”
src/types/fetch.ts 97.29% <100.00%> (ΓΈ)
src/types/index.ts 100.00% <100.00%> (ΓΈ)
src/types/utils.ts 100.00% <100.00%> (ΓΈ)
src/imports.ts 95.23% <0.00%> (ΓΈ)
src/presets/heroku.ts 100.00% <0.00%> (ΓΈ)
src/presets/nitro-dev.ts 100.00% <0.00%> (ΓΈ)
src/rollup/plugins/public-assets.ts 88.23% <0.00%> (ΓΈ)
src/scan.ts 39.47% <0.00%> (ΓΈ)
src/presets/azure.ts 13.70% <0.00%> (ΓΈ)
src/options.ts 85.93% <0.00%> (ΓΈ)
... and 46 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data
Powered by Codecov. Last update 7aaab6d...3622bb6. Read the comment docs.

@pi0
Copy link
Member

pi0 commented Aug 1, 2022

Thanks @didavid61202 @danielroe <3

@pi0 pi0 merged commit 082d58f into unjs:main Aug 1, 2022
WinterYukky pushed a commit to WinterYukky/nitro that referenced this pull request Nov 1, 2022
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.

Should we advocate Volar over Vetur?
3 participants