don't call FETCH_SUCCESS on error

[federicoweber]

This is needed so that in case of errors we can handle it in the FETCH_FAIL handler, without worrying about protecting the FETCH_SUCCESS logic from missing or malformed data.

This is needed to properly handle RPC errors in the front-end in https://github.com/bufferapp/buffer-analyze/pull/337

Hey @djfarrelly and @msanroman, would love your eyes on this one, not sure if I'm missing any cases where we want to triggerFETCH_SUCESS in case of errors.

[federicoweber]

Hey @msanroman and @djfarrelly,
doing a bit more digging I think the source of the double triggered event is micro-rpc-client specifically the logic in https://github.com/bufferapp/micro-rpc-client/blob/master/index.js#L26-L50.

If I'm reading this correctly:

the first .then will always return a Promise that is correctly resolved, and this will trigger the SUCCESS event. As we are passing the error in the body the catch won't fire, unless there is a problem parsing the response.

 .then(
        response =>
          new Promise((resolve, reject) => {
            response
              .json()
              .then(parsedResponse =>
                resolve({
                  response: parsedResponse,
                  status: response.status,
                }),
              )
              .catch(error => reject(error));
          }),
      )

and the second one will find the error and trigger the FAIL event.

      .then(({ response, status }) => {
        if (response.error) {
          const err = new Error(response.error);
          err.code = response.code;
          err.handled = response.handled;
          err.status = status;
          throw err;
        }
        return response;
      })

I think it will be enought to switch the two and have the error check happen first, but I have't tested that yet.

Closing this PR, as is not really solving the root cause.

[djfarrelly]

Should we remove this debugging line? If you find this logging useful for you, maybe add an option to the middleware like { debug: true }?

[djfarrelly]

I'm curious, what is the response code in this case? My only question is, should this be handled in the RPCClient library and not here? If you think this should be handled higher up in the client library, that might be good to ship a change there first, but that's your call. If you want to bounce an idea around for that, I know Eduardo and Phil have thought a bunch about the RPC server side code and the client itself and we haven't iterated on the client library since Harrison wrote it last year :D

[djfarrelly]

Whoops - sorry @federicoweber, I missed your update on this before my comments 😆 - It looks like we're on the same page already! 😉 Def connect w/ Edu & Phil if you want to bounce around more ideas or are looking for reviews!