Skip to content
This repository has been archived by the owner on Apr 6, 2023. It is now read-only.

refactor(nuxt): enhance useFetch and useLazyFetch request type #4825

Merged
merged 28 commits into from
Jul 25, 2022

Conversation

didavid61202
Copy link
Contributor

@didavid61202 didavid61202 commented May 5, 2022

Updates


Enhance the useFetch and useLazyFetch request's type by inferring nitropack's server route's string literal (InternalApi interface). supporting auto-complete and showing type hints.

🔗 Linked issue

Resolve nuxt/nuxt#13934

❓ 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

Enhance the useFetch and useLazyFetch request's type by inferring nitropack's server route's string literal (InternalApi interface).

This will provide better DX and prevent typos while using useFetch and useLazyFetch by supporting auto-complete and showing type hints when entering the request. And should still be able to pass other valid request URLs like https://example.com/api or Request object like new Request('/api/test')

Usage scenario

assume project contains server/api/test.ts

import type { NitroFetchRequest } from 'nitropack'

const { data } = useFetch(/** IDE should show hint: '/api/test' */)

const request: NitroFetchRequest = /** IDE should show hint: '/api/test' */
useFetch('/api/test') // ok
useFetch('https://example.com/api') // ok
useFetch(new Request('api/test')) // ok

useFetch(/** will not show internal routes ex: '__nuxt_error' */)

📝 Checklist

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

Enhance the useFetch and useLazyFetch request's type by inferring nitropack's server route's string literal (InternalApi interface). supporting auto-complete and showing type hints.
@netlify
Copy link

netlify bot commented May 5, 2022

Deploy Preview for nuxt3-docs ready!

Name Link
🔨 Latest commit 25726e1
🔍 Latest deploy log https://app.netlify.com/sites/nuxt3-docs/deploys/62da875f84a02500099b8754
😎 Deploy Preview https://deploy-preview-4825--nuxt3-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

…zyFetch` #4823

add type tests to composables suite, calling useFetch with: 1. all routes generated from server, 2. external url, 3. Request object. And should throw type error when pass in invalid request string
@danielroe danielroe added enhancement New feature or request dx 🍰 p2-nice-to-have Priority 2: nothing is broken but it's worth addressing labels May 5, 2022
@Rigo-m
Copy link
Contributor

Rigo-m commented May 5, 2022

One thing that's missing is the ability to type POST request body, is there a way to implement it?

@didavid61202
Copy link
Contributor Author

One thing that's missing is the ability to type POST request body, is there a way to implement it?

@Rigo-m I'll look into this!

@Rigo-m
Copy link
Contributor

Rigo-m commented May 5, 2022

Sorry that I can't help, I'm still bad a TS somehow hahaha

@danielroe danielroe self-requested a review May 5, 2022 18:59
@didavid61202
Copy link
Contributor Author

didavid61202 commented May 5, 2022

Sorry that I can't help, I'm still bad a TS somehow hahaha

Got a working code that infers the body type of a POST API, but still needs to manually export the type from the API file and change the auto-generated InternalApi interface, or have to make some change to nitropack (and maybe h3) to make the implementation cleaner.
I would have to discuss this with @danielroe or other core team members about this for code organization and naming consistency. then do another issue/PR.

hacky method

// .nuxt/types/nitro.d.ts
declare module 'nitropack' {
  type Awaited<T> = T extends PromiseLike<infer U> ? Awaited<U> : T
  interface InternalApi {
// turn value of InternalApi in to tuple, with index 1 being the body type
    '/api/test': [Awaited<ReturnType<typeof import('../../server/api/test').default>>, import('../../server/api/test').BodyType]
    '/__nuxt_error': [Awaited<ReturnType<typeof import('../../../packages/nuxt/src/core/runtime/nitro/renderer').default>>,'']
  }
}

// server/api/test.ts
export type BodyType = {tag:string, index:number } // export the manually defined type

export default defineEventHandler(async (event) => {
  const body = await useBody<BodyType>(event)
  return { result: body.tag }
})

// composable/fetch.ts
export declare type FetchRequestUrl =
  Exclude<keyof InternalApi, '/__nuxt_error'>
  | (`${string}${'/'|'.'}${string}` & {})
  | Exclude<FetchRequest, string>
interface ExFetchOptions<Route extends FetchRequestUrl> extends FetchOptions {
  body?: Route extends keyof InternalApi ? InternalApi[Route][1] : FetchOptions['body']
}
export interface UseFetchOptions<
  DataT,
  ReqT extends FetchRequestUrl,
  Transform extends _Transform<DataT, any> = _Transform<DataT, DataT>,
  PickKeys extends KeyOfRes<Transform> = KeyOfRes<Transform>
> extends AsyncDataOptions<DataT, Transform, PickKeys>, ExFetchOptions<ReqT> {
  key?: string
}

// usage
const { data } = useFetch('/api/test', { // have to update nitropack's type to correctly infer data type
  method: 'POST',
  body: { tag: '3', index: 2 } // body will be type 'BodyType '
})

@Rigo-m
Copy link
Contributor

Rigo-m commented May 5, 2022

Maybe a clean solution could be manually typing body by extending nuxt schema like we do for manually tiping useRuntimeConfig?

@danielroe
Copy link
Member

I love this idea. I think rather than seeking to implement within useFetch within Nuxt we should do so within nitropack which is where the rest of the $fetch inference comes from: https://github.com/unjs/nitro/blob/4fbeef9bfd5c8e064817865a31d61927addc5e2a/src/types/fetch.ts

... and then update this PR accordingly.

WDYT @pi0?

@pi0
Copy link
Member

pi0 commented May 6, 2022

I also love to try this idea! Indeed nitro seems better place to implement types for $fetch.

@pi0 pi0 added the pending label May 6, 2022
@didavid61202
Copy link
Contributor Author

I love this idea. I think rather than seeking to implement within useFetch within Nuxt we should do so within nitropack which is where the rest of the $fetch inference comes from: https://github.com/unjs/nitro/blob/4fbeef9bfd5c8e064817865a31d61927addc5e2a/src/types/fetch.ts

... and then update this PR accordingly.

WDYT @pi0?

Sounds great! I implement it in Nuxt first as I wasn't sure if this should be implemented in nitropack. But after some digging and trying to implement type hint for POST body yesterday, I also found out that the codes to updates are mostly in nitropack.

mainly:

  1. move this PR's new FetchRequestUrl type into nitropack
  2. the structure of the InternalApi interface: except for the default return of server function, we can also expose the request's body type...etc as a Map type for future expansion? (as name export in server files or update defineEventHandlers signature)

I'll find some time to dig into it 👍

@didavid61202
Copy link
Contributor Author

didavid61202 commented May 6, 2022

I love this idea. I think rather than seeking to implement within useFetch within Nuxt we should do so within nitropack which is where the rest of the $fetch inference comes from: https://github.com/unjs/nitro/blob/4fbeef9bfd5c8e064817865a31d61927addc5e2a/src/types/fetch.ts
... and then update this PR accordingly.
WDYT @pi0?

Sounds great! I implement it in Nuxt first as I wasn't sure if this should be implemented in nitropack. But after some digging and trying to implement type hint for POST body yesterday, I also found out that the codes to updates are mostly in nitropack.

mainly:

  1. move this PR's new FetchRequestUrl type into nitropack
  2. the structure of the InternalApi interface: except for the default return of server function, we can also expose the request's body type...etc as a Map type for future expansion? (as name export in server files or update defineEventHandlers signature)

I'll find some time to dig into it 👍

Hey, @danielroe @pi0 would like to get some suggestions/notices (if any) on point 2 before I start, for changing the InternalApi interface and defineEventHandlers signature.
And if we want to auto infer Body type by using useBody, we might also have to update h3?
Thanks in advance ❤️

@didavid61202
Copy link
Contributor Author

Update

Implementing this enhancement in unjs/nitro repo first (nitrojs/nitro#208), and will update this PR accordingly.

@pi0
Copy link
Member

pi0 commented May 10, 2022

@didavid61202 Since nitrojs/nitro#208 is landed, do you mind to rework this PR? Thanks!

@didavid61202
Copy link
Contributor Author

I'm on it! 🚀

@pi0
Copy link
Member

pi0 commented May 12, 2022

and I was wondering if we should exclude the /__nuxt_error path from NitroFetchRequest, or still let user sees it?
maybe we could also rename and export the Request type by FetchRequest or NuxtFetchRequest, any suggestion? 😃

I would hide all /_* and /api/_* routes from auto completion as they are internal.

@didavid61202
Copy link
Contributor Author

didavid61202 commented May 12, 2022

and I was wondering if we should exclude the /__nuxt_error path from NitroFetchRequest, or still let user sees it?
maybe we could also rename and export the Request type by FetchRequest or NuxtFetchRequest, any suggestion? 😃

I would hide all /_* and /api/_* routes from auto completion as they are internal.

updated in b49385c, excluding all /_* and /api/_* routes and export/alias NitroFetchRequest to FetchRequest

@pi0
Copy link
Member

pi0 commented May 12, 2022

@didavid61202 Maybe we can move this fix directly to Nitro and avoid workaround? I can quickly merge it for nitro and this after that :)

@didavid61202
Copy link
Contributor Author

didavid61202 commented May 12, 2022

@didavid61202 Maybe we can move this fix directly to Nitro and avoid workaround? I can quickly merge it for nitro and this after that :)

sure, on it!
and how is the alias's nameing? adding Nuxt prefix? or keep it as FetchRequest?

@danielroe
Copy link
Member

Looking great! There were a couple of workarounds in 7e89fe8 that I think we can now remove as well, if you're up for it 😊

@pi0
Copy link
Member

pi0 commented May 12, 2022

and how is the alias's nameing? adding Nuxt prefix? or keep it as FetchRequest?

Can we directly use NitroFetchRequest everywhere? It reduces chances of inconsistent typings.

@pi0 pi0 closed this May 12, 2022
@pi0 pi0 reopened this May 12, 2022
@didavid61202
Copy link
Contributor Author

didavid61202 commented May 12, 2022

sure, on it! will see what I can do 👍

@didavid61202
Copy link
Contributor Author

didavid61202 commented May 12, 2022

and how is the alias's nameing? adding Nuxt prefix? or keep it as FetchRequest?

Can we directly use NitroFetchRequest everywhere? It reduces chances of inconsistent typings.

got it!
@pi0 fixed in nitrojs/nitro#232 👍

@danielroe danielroe changed the title feat(nuxt): enchance useFetch and useLazyFetch request type #4823 feat(nuxt): enhance useFetch and useLazyFetch request type #4823 May 12, 2022
@danielroe danielroe changed the title feat(nuxt): enhance useFetch and useLazyFetch request type #4823 feat(nuxt): enhance useFetch and useLazyFetch request type May 12, 2022
@danielroe danielroe requested a review from pi0 July 22, 2022 11:27
@pi0 pi0 removed the pending label Jul 25, 2022
@pi0 pi0 changed the title feat(nuxt): enhance useFetch and useLazyFetch request type refactor(nuxt): enhance useFetch and useLazyFetch request type Jul 25, 2022
@pi0 pi0 merged commit 3a822c7 into nuxt:main Jul 25, 2022
@pi0
Copy link
Member

pi0 commented Jul 25, 2022

Thanks for working on this @didavid61202 @danielroe <3

@pi0 pi0 mentioned this pull request Aug 5, 2022
@danielroe danielroe added the 3.x label Jan 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
3.x dx enhancement New feature or request 🍰 p2-nice-to-have Priority 2: nothing is broken but it's worth addressing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: useFetch request type infer from nitro's server routes (keyof InternalApi)
4 participants