davnicwil / react-frontload

Async data loading for React components, on client & server
451 stars 21 forks source link

Error code #53

Open shotap opened 3 years ago

shotap commented 3 years ago

Hi, I think that the frontloadMeta should return some data about the error and not only error: true when we do SSR there is a big difrance between HTTP 500 or HTTP 404, especially when talking about google crawl budget. I tried to find some data about the error and all i front is the error flag. maybe return an array of errors where it will hold all the errors in the useFrontload?

davnicwil commented 3 years ago

Interesting idea, but I'll try to convince you that this should be left as it is.

Having error be a boolean flag was a deliberate design choice.

One of the nice things about having useFrontload just wrap a function inline in your Component is that it's just Javascript, has access to all your component's props and state, and so can do anything you want.

It means the scope of useFrontload can be quite small - it just provides some conveniences like data state and a setter, isLoading state, and the error flag, so that you don't have to manually plumb these in every time in your function. Other than that, though, everything should just be handled by your app.

On the error flag specifically - the behaviour is that the first thrown and uncaught error in your function is swallowed on the client, and this flag is set true. You could then use it to show some generic error message component, for example. It's just a convenience - so that you don't have to wrap all your useFrontload function implementations in try catch and manually plumb in some error state, just to make sure you always handle uncaught errors.

But for anything more advanced, like dealing with error codes, collecting multiple errors in an array, etc, the idea is that you do this in the function in useFrontload - not using any error object it would return.

davnicwil commented 3 years ago

For example, suppose that we had the error object, then you could do this:

const MyComponent = () => {
  const { data, frontloadMeta } = useFrontload('my-component', async ({ api }) => ({
    stuff: await api.getStuff()
  }))

  // handle errors
  if (frontloadMeta.error.code === 500) {
    // do stuff
  } else if (frontloadMeta.error.code === 404) {
    // do something else
  }

  ...

But the idea is that, instead, you should just move that error handling code inside the useFrontload - that way it doesn't need to return the error object.

const MyComponent = () => {
  const { data, frontloadMeta } = useFrontload('my-component', async ({ api }) => (
    try {
      return { stuff: await api.getStuff() }
    } catch (err) {
      // do error handling here instead
      if (err.code === 500) {
        ...
    }
  })
shotap commented 3 years ago

I generally agree, but we have a lot of use of frontload, around 450 of them in our app. Applying this method will add a lot of code for us

My idea was to catch all the errors and return an array of errors and let the components decide what to do with them.

davnicwil commented 3 years ago

Ah, sure. Having thought about this, I think adding the error object to frontloadMeta is a pretty pragmatic and useful change.

Reviewing the two examples above, actually the first way is a lot simpler in many ways, especially if you are just doing things like checking codes. What I wanted to avoid was expanding the scope to do any 'error handling' type stuff, but actually just returning the error object is basically just the same as returning a boolean but more flexible and useful!

I'd propose adding an additional errorObject field to not make a breaking change to the existing API though. Would be very happy to accept a PR for this, but if not I'll probably get it done at some point over the next couple of weeks.

shotap commented 3 years ago

https://github.com/davnicwil/react-frontload/pull/54

davnicwil commented 3 years ago

So first of all, sorry for the slow turnaround on this. It's been a very busy January!

I've been thinking about the solution proposed in your PR . Reviewing this actually made me realise the reason I'd made the error a boolean flag in the first place.

The problem with returning the error object as is is serialization - rehydrating it on the client after the server render. An error thrown on the server would have to be serialized in some way so that all the info you use from it to render things in your components can also be passed to the client.

I think a fairly elegant solution to this is the same pattern used by many web frameworks - the ability to configure a global error handler for frontload that can do this job. That is, you set it once, and then any frontload that runs and throws an Error, on server or client, would run through this error handler automatically. So you don't have to write handler code in each one of your frontloads, and you can serialize whatever info you need from each type of error and return it in (probably) the errorObject.

What do you think?

shotap commented 3 years ago

Hi, Maybe we should do something like this:

let cache: any[] = [];
JSON.stringify(data, (key, value) => {
    if (typeof value === 'object' && value !== null) {
        if (cache.includes(value)) return;
        cache.push(value);
    }
    return value;
})
cache = null;