nuxt-directus icon indicating copy to clipboard operation
nuxt-directus copied to clipboard

feat: normalize errors

Open sandros94 opened this issue 1 year ago • 9 comments

I've yet to test this, but instead of doing a bunch of console.errors I could serialize any error coming from Directus.

Will update once I have time to work on this

sandros94 avatar Feb 10 '24 13:02 sandros94

Hello, still me here :). I updated to latest version of the module to try evolutions and so on ("^0.0.10"). Default user profile request works like a charm, thanks a lot for this!

I have questions/concerns about errors management so it might be the right place here.

Concerning errors management for the login method: I see that the useDirectusAuth().login method catch all errors, print them in the console and return a null value:

try {
   const authResponse = await client
     .request(sdkLogin(identifier, password, defu(options, { mode: defaultMode })))

   if (updateStates !== false) {
     if (updateTokens !== false) {
       await setTokens(authResponse)
     }
     if (readMe !== false) {
       await readMyself(readMe)
     }
   }

   return authResponse
 } catch (error: any) {
   if (error && error.message) {
     console.error("Couldn't login user.", error.message)
   } else {
     console.error(error)
   }
   return null
 }
}

I don't know if/how you have planned to change this but I think errors causes/details have to be thrown in one way or another, because we need to know for example that the password is wrong, based on returned payload or that the server is down or whatever kind of error has happened to warn the user about it and/or handle the issue.

This behaviour seems to be also found in other methods like useDirectusItems().createItem where I can also only 'blindly' throw a message and not analyse and handle it properly based on returned response if any.

Why not just throwing the raw error instead of replying with null for the moment in all final catch methods?

Is the h3's createError evolution planned to take care of all this?

zguig52 avatar Feb 22 '24 08:02 zguig52

Is the h3's createError evolution planned to take care of all this?

Exactly, but since I wanted to investigate a little bit more how errors are generated in the SDK I decided to postpone it to the next minor release after the stable 6.0.x was made

sandros94 avatar Feb 22 '24 09:02 sandros94

I just found a very very annoying use case due to the fact of not throwing errors: ==> useDirectusItems().deleteItem

I have exactly the same behavior, that it work or not, as the return value is undefined either if it works or not...

I am not able to handle, even blindly the deletion error.

zguig52 avatar Feb 23 '24 10:02 zguig52

And the same might happen for "dark hole" items, where you only have creation permissions and no read permission, as Directus will reply with a 204 without any body.

zguig52 avatar Feb 23 '24 10:02 zguig52

[...] as the return value is undefined either if it works or not...

I am not able to handle, even blindly the deletion error.

there are a number of functions from the SDK, in particular the delete* and logout that do return nothing, requiring a doublecheck via other means if that item was successfully deleted.

On the other hand, while working on useAsyncData support I've noticed that the number of try-catch blocks that I had to use before being able to use $fetch was still there. And since I'm using $fetch directly from Nuxt I don't have to worry about adding nor managing errors caused by fetch itself (nor retries or similar).

I've already did commit some changes on this topic, but I'll need to finish some other few before releasing a new nuxt-directus-next update. I'll update this issue once I do.

Side effect

The SDK has a few errors that are not currently properly handled, blocking UX. Since I see an open issue on this (directus/#21474) I might not just map each error one by one and wait for any update on that side (or at least until we are ready for a stable 6.0 release).

sandros94 avatar Feb 23 '24 20:02 sandros94

@zguig52 I've updated nuxt-directus-next to 0.0.11, here as #215 (comment) you can find some notes about the changes.

Now we should be able to handle all the errors caused during a fetch, with the only handful of generic errors generated by the SDK for things like missing IDs or trying to operate on core collections.

Based on the ETA and direction of the Directus issue I'll either close this for now or leave it open, but feel free to write if you encounter any error that could be caused by me (in that case remember to provide a reproduction).

sandros94 avatar Feb 24 '24 03:02 sandros94

That's fantastic, thanks a lot for this! Here are my feedbacks and tests based on 0.11 release. I've been mainly playing with directus permissions (403 + data with errors details from directus) and server stop (proxy in front, empty 503 reply).

Included in a try catch:

  • useDirectusAuth().login OK
  • useDirectusAuth().logout OK
  • useDirectusFiles().deleteFiles OK
  • useDirectusFiles().uploadFiles OK
  • useDirectusItems().deleteItem OK
  • useDirectusItems().readItem OK

Usage:

try{
METHOD CALL HERE
}catch(error: any){
  console.error("functionName", error)
  displayMessage("error", error.data?.errors[0]?.message ?? error) // whatever you want to process the error
}

With async syntax (const { data, error}):

  • useDirectusItems().readAsyncItems OK

Usage:

    const { data, error } = await useDirectusItems<Collections>().readAsyncItems(collection, {
      query: {
        fields: fields
      }
    })
    if(!error.value && data.value){
      return data.value;
    }else{
      console.error("getExistingRegistration", error.value)
      displayMessage("error", error.value.data?.errors[0]?.message ?? error) // whatever you want to process the error
    }

One remark, when using the readItems, query parameter signature are a bit different from the readAsyncItems: readItems(collection, {fields: ['']}) (has changed with recent version if I remember well - was like the async signature today before) readAsyncItems(collection, {query: {fields: ['']}}) (did not put the ref stuff to make the difference more visible) ==> It might be better to align both with the same query syntax for easier use, don't you think?

BTW, thanks for the example on how to type the different parameters of functions in your release notes, it helped me a lot to remove many //@ts-ignore :)!

zguig52 avatar Feb 24 '24 17:02 zguig52

Thanks for testing things out! Much appreciated 🔥

One remark, when using the readItems, query parameter signature are a bit different from the readAsyncItems: readItems(collection, {fields: ['']}) (has changed with recent version if I remember well - was like the async signature today before) readAsyncItems(collection, {query: {fields: ['']}}) (did not put the ref stuff to make the difference more visible) ==> It might be better to align both with the same query syntax for easier use, don't you think?

Are you referring to the fact that readAsync** require the query key?

... readItems('posts', { fields: ['*'] })
// compared to
... readAsyncItems('posts', { query: { fields: ['*'] } })

If yes then this is intentional for a number of reasons:

  • it becomes simpler to handle the reactivity for keys inside query (something needed to prevent issues like the one in #238)
  • the first follows signatures from the SDK while the latter Nuxt's ones (see useFetch)
  • it prevents you from breaking Geneva agreement by doing something like:
    ... readAsyncItems('todos', {}, { server: false })  🤮
    ... readAsyncItems('todos', { server: false })      👌 // current implementation
    

BTW, thanks for the example on how to type the different parameters of functions in your release notes, it helped me a lot to remove many //@ts-ignore :)!

Yeah, once all the implementations and bugs are settled I would like to start filling the new docs with tons of examples. It's way easier to see how to use the tool instead of reading how it should be used.

Currently there are still some places where @ts-ignore is still kinda required (within readAsync** functions), but those are semi-advanced use cases where manually using useAsyncData+readI** is much easier on Nuxt and TS engines.

BTW: this update also let you use defaults of any if no schema is passed to the main composables, meaning that things like fields fall back to normal string[] types.

<script setup lang="ts"> // using ts
const { readAsyncItems } = useDirectusItems() // no schema type passed

const fields = ref(['id', 'title']) // normal `string[]` type

const { data } = await readAsyncItems('posts', {
  query: {
    fields // accepts the `Ref<scring[]>` type
  },
  watch: [fields]
})
</script>

sandros94 avatar Feb 25 '24 00:02 sandros94

If yes then this is intentional for a number of reasons:

Thanks for the clarifications!

zguig52 avatar Feb 26 '24 09:02 zguig52