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: add useStrapi composable with correct types #279

Merged

Conversation

harlan-zw
Copy link
Contributor

@harlan-zw harlan-zw commented Sep 2, 2022

Types of changes

  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

This PR aims to fix broken types without introducing breaking changes.

See:

To summarise the type issues:

  • single/multiple return requests have ambiguous types
  • it's not possible to type the update / create functions without an error

To fix the types without breaking anything, I've introduced a new composable useStrapi which is a simple wrapper to the existing v3 or v4 client, depending on which version is being used, with the correct types.

The types for the client allow a user to either set a default type for all requests from the composable useStrapi<MyData> and / or override the single data return types from the functions find<Book>('book')

useStrapi3 and useStrapi4 are deprecated in favour of this to notify users they should swap over, this can be removed but I think it's worth while to have a single composable in the docs which have correct types

I believe these changes are a huge DX improvement:

  • No type importing
  • Single and multiple return types are automatically handled
  • Proper types for CRUD
  • Single composable, makes upgrading easier

While this shouldn't break anything, it should be a minor bump and simple migration docs should be included in the release

⚠️ Note

While these changes have minimal scope for breaking things there are no test cases and I've never used strapi, so ideally someone would test this PR locally before merging.

Migration

Check the docs for latest type guidelines.

Previous version

import type { Strapi4Response } from '@nuxtjs/strapi'
const { find } = useStrapi4()
const response = await find<Strapi4Response<Restaurant>>('restaurants')

New version

const { find } = useStrapi()
const response = await find<Restaurant>('restaurants')

or

const { find } = useStrapi<Restaurant>()
const response = await find('restaurants')

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes (if not applicable, please state why) - no test suit

@benjamincanac
Copy link
Member

Hello @harlan-zw, thanks for your PR it's awesome! I've made two commits to fix the lint and removed a deprecated, can you let me know what you think? Otherwise the PR is good for me! :)

@harlan-zw
Copy link
Contributor Author

Thanks for the review @benjamincanac!

Commits look good, I'm not too fussed about the removal of the @deprecated flag, it just acts as a way to push people towards using the function with the correct types, without introducing breaking changes. If it wasn't obvious, useStrapi is the same as useStrapi{version})

Did you test this locally with an actual strapi API by any chance? I would have liked to add tests for it but it was missing a suite and was too much work to add it.

@benjamincanac
Copy link
Member

I removed the deprecated on useStrapi composable not useStrapi3, I'll test this on my projects once we merge it through the edge channel!

@benjamincanac benjamincanac merged commit 43f76a7 into nuxt-modules:dev Sep 27, 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.

None yet

3 participants