cozy / cozy-client

Document store and React components for the Cozy platform
MIT License
13 stars 13 forks source link

Handle error and deal with UI in cozy-client ? #552

Open Crash-- opened 4 years ago

Crash-- commented 4 years ago

Let's say we have an application uploading a few files. This application will use Cozy-Client and specially FileCollection.createFile() to upload the file.

Currently, cozy-stack returns an HTTP error 413 if the cozy is out of free space and cozy-client throws this error. That mean that, in my application I need to do something like :

try {
  const upload = await filecollection.createFile()
} catch(error){
  if(error.status === 413){
    showModal(<QuotaAlert />
  }
}

It works well if we have only one application handling the upload, we'll though about this case and implement it. But this is not the case anymore. We have several apps or libs that deal with uploading files, and we need to implement this logic everywhere.

We should mutualize this work and be sure, that even if the application doesn't handle the 413 error, we still display this QuotaAlert.

Should we create a way to catch cozy-client exception in a plugin and instantiate this plugin in the cozy-client Provider ? Something like:

Provider

constructor(){
  //...
  client.registerPlugin(ExceptionsPlugin);
}

ExceptionPlugin:

import {showModal, QuotaAlert} from 'cozy-ui'

class ExceptionPlugin {

constructor(client){
  client.on('exception', this.handleException)
}

handleException = (e) => {
  if(e.status === 413){
    showModal(<QuotaAlert />)
  }
}
ptbrowne commented 4 years ago

I fully agree, we should have generic errors handled automatically.

I am not sure a plugin should be used here as it is not made for React, a plugin does not live inside the React tree. A component could fit the job here I think.

const CozyProvider = props => (
  <Provider>
    { props.children }
    <ClientErrors client={client} />
  </Provider>
)
Crash-- commented 4 years ago

How this ClientErrors can listen an exception if there is no plugin dispatching the event? What do you have in mind?

edas commented 4 years ago

I'm not fond of having a generic exception plugin that catch all 413 requests. As I see it, UI response should be dealt by UI components, not by dev APIs.

May we add this to the weekly meeting tomorow ?

ptbrowne commented 4 years ago

How this ClientErrors can listen an exception if there is no plugin dispatching the event? What do you have in mind?

cozyClient would emit "exception" events and ClientErrors would start listening to this event in its componentDidMount.

ptbrowne commented 4 years ago

As I see it, UI response should be dealt by UI components, not by dev APIs.

If I understand well, you propose that the QuotaAlert component be explicitly shown to the user : the app dev should think of the upload failure and show it if there is an error.

What I see as an advantage:

Inconvenients:

An alternative would be :

const handleClientError = err => isQuotaAlert(err) && showQuotaAlert(err)

try {
  client.doUpload()
} catch (e) {
  handleClientError(e)
}
edas commented 4 years ago

We could even have a

import { quotaErrorUi, errorsUi } from 'cozy-client'

const managedErrors = [ quotaErrorUi ]

try {
  client.doUpload()
} catch (e) {
  if (! errorsUI(e, managedErrors ) ) {
    // errors not manager by one of our default handlers
  } 
}

errorsUI will do what you suggest (check if this is a quota error and launch the quota modal if so).

This way we could define a lot of handlers in cozy-client, but still have them pushed explicitly, and letting the caller define their own or opt-out for what they want.