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: normalize errors #243

Open
sandros94 opened this issue Feb 10, 2024 · 9 comments
Open

feat: normalize errors #243

sandros94 opened this issue Feb 10, 2024 · 9 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@sandros94
Copy link
Collaborator

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 sandros94 added the enhancement New feature or request label Feb 10, 2024
@sandros94 sandros94 self-assigned this Feb 10, 2024
@sandros94 sandros94 added this to the v6.1.x milestone Feb 10, 2024
@zguig52
Copy link

zguig52 commented Feb 22, 2024

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?

@sandros94
Copy link
Collaborator Author

sandros94 commented Feb 22, 2024

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

@zguig52
Copy link

zguig52 commented Feb 23, 2024

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
Copy link

zguig52 commented Feb 23, 2024

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.

@sandros94
Copy link
Collaborator Author

[...] 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
Copy link
Collaborator Author

sandros94 commented Feb 24, 2024

@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).

@zguig52
Copy link

zguig52 commented Feb 24, 2024

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 :)!

@sandros94
Copy link
Collaborator Author

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 reactive query parameters with built-in useAsyncData #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>

@zguig52
Copy link

zguig52 commented Feb 26, 2024

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

Thanks for the clarifications!

@sandros94 sandros94 modified the milestones: v6.1.x, v6.x, v6.0.x Feb 26, 2024
@sandros94 sandros94 changed the title feat: normalize errors with h3's createError feat: normalize errors Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants