logaretm / villus

🏎 A tiny and fast GraphQL client for Vue.js
https://villus.dev
MIT License
790 stars 31 forks source link

feat: add onSuccess and onError hooks in useQuery function #190

Closed jbaubree closed 1 year ago

jbaubree commented 1 year ago

Fixes #189

@logaretm i hope this will be helpful, feel free to update this PR as needed or remove it if you think this is not a wanted feature.

logaretm commented 1 year ago

Interesting addition, I don't mind since its not really a big bundle increase and maybe is a better API than people having to watch data and error although that is what I have been doing.

Can you add tests to this?

codecov-commenter commented 1 year ago

Codecov Report

Merging #190 (2e3152a) into main (3b03942) will increase coverage by 0.02%. The diff coverage is 100.00%.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main     #190      +/-   ##
==========================================
+ Coverage   93.98%   94.01%   +0.02%     
==========================================
  Files          21       21              
  Lines         549      551       +2     
  Branches      124      126       +2     
==========================================
+ Hits          516      518       +2     
  Misses         33       33              
Impacted Files Coverage Δ
packages/villus/src/useQuery.ts 97.64% <100.00%> (+0.05%) :arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

jbaubree commented 1 year ago

Thanks @logaretm, i just added tests for this hooks 👌

jbaubree commented 1 year ago

Well, what you are thinking about is good. I personally think that the need to pass to other function or anything around this are each cases and can simply be override with a composable. I like the way we can write code with the integration i wrote because of two reasons:

// All refs
const items = ref<User[]>([])
const nbItems = ref(0)

// All computed
const skip = computed(() => (currentPage.value - 1) * itemsPerPage.value)
const getUsersQuery = computed(() => USERS_GET_QUERY(skip.value)

// All utilities with impacts on refs
const { data, loading } = useQuery<{ users: UserCollectionSegment; roles: Role[] }>({
  query: getUsersQuery,
  onSuccess: ({ users, roles }) => {
    items.value = users.items ?? []
    nbItems.value = users.totalCount
  },
})
const { execute: updateUser } = useMutation<{ editUser: User }>({
  query: USER_EDIT_MUTATION,
  onSuccess: () => {
    notificationBus.emit({ variant: 'success', title: '.............' })
  },
})

// functions
function onRowClick(user: User) {
  ...
}
function onCheckboxClick(id: string) {
  ...
}

etc...

I'm using this way to write code because i'm using VueUse too and useAxios is wrote like this.

I totally agree with you with the fact that your proposition is a really good way too. If you think is will be better, it will be ok for me 👍

EDIT: in addition, the actual then(onFulfilled) function is doing what you want (sure without specific onSuccess/onError). If we keep my way and then function, developer can choose the way he prefers to use. In the other case, if you make onSuccess and onError hooks with your method, i think then function can be deleted.

logaretm commented 1 year ago

Yep, it always can be added later without breaking changes so all good, will tag this in a release now. Thank you for the contribution!