relay-tools / react-relay-network-layer

ReactRelayNetworkLayer with middlewares and query batching for Relay Classic.
MIT License
277 stars 47 forks source link

Throwing errors from middleware #35

Closed ryanmcclure4 closed 7 years ago

ryanmcclure4 commented 7 years ago

I'm trying to handle errors from mutations similarly to what is described here, and so I'm passing an errors object in my mutation payload. These errors are then handled in the onSuccess callback of the mutation. This presents an issue because to relay it appears as though the mutation completed successfully, and so the relayRequest is resolved rather than rejected. Sometimes this is ok, but in the case of RANGE_ADD type relay mutations, relay proceeds to add an edge when there is no newly created edge.

I'm able to solve this by modifying the mutation function as such:

  return fetchWithMiddleware(req)
    .then(payload => {
      if (payload.hasOwnProperty('errors')) {
        const error = new Error(
          'Server request for mutation `' + relayRequest.getDebugName() + '` ' +
          'failed for the following reasons:\n\n' +
          formatRequestErrors(relayRequest, payload.errors)
        );
        error.source = payload;
        relayRequest.reject(error);
      } else {
        Object.keys(payload.data).forEach(key => {
          const errors = payload.data[key].errors;
          if (errors && errors.length > 0) {
            const error = new Error('Server request failed');
            error.source = errors;
            relayRequest.reject(error);
          } else {
            relayRequest.resolve({ response: payload.data });
          }
        })
      }
    }).catch(
      error => relayRequest.reject(error)
    );

But I realized that rather than modifying this code I should be able to just add a custom middleware, which would throw an error for the mutation function's fetchWithMiddleware to catch:

next => req => {
  const resPromise = next(req);

  if (req.relayReqType === 'mutation') {
    resPromise.then(res => {
      Object.keys(res.json.data).forEach(key => {
        const errors = res.json.data[key].errors;
        if (errors && errors.length > 0) {
          const error = new Error('Server request failed');
          error.source = errors;
          throw error;
        }
      });
    });
  }

  return resPromise;
}

However the fetchWithMiddleware never catches this error, proceeds to resolve the relayRequest, and I get an Unhandled promise rejection error in the console.

Am I maybe misunderstanding the nature of middleware? Is there a better way I might go about doing this?

ryanblakeley commented 7 years ago

I have an issue too where I think fetchWithMiddleware is not being called in mutation. I'm using the option {url: _ => '/appname-graphql'}. A fetch for a query honors the setting and works. A fetch for a mutation is defaulting to /graphql and breaking the app.

nodkz commented 7 years ago

Wow, completely miss this issue. I'm so sorry. 😞

@ryanmcclure4 you may try version 2.0.0 there was rewritten error catching logic 🎉. And your custom solution with middleware should work. Your case is covered by this test https://github.com/nodkz/react-relay-network-layer/blob/master/test/mutation.test.js#L36-L54

@rojobuffalo you are talking about a different problem, please open a new issue with your full config. I think that problem somewhere there. I'm just covered urlMidleware with tests https://github.com/nodkz/react-relay-network-layer/blob/master/test/middleware/url.test.js and mutations in it works perfectly with urlMiddleware.


@ryanmcclure4 again sorry for late answer. I'm closing this issue, but fill free to reopen it if the problem stays. But please hurry with error reporting, you may again catch me when I'll be on vacation (from 25.03 till 16.04.2017) and in the second time get a late response from me. 😥

ryanblakeley commented 7 years ago

@nodkz I just found the fix, and I'm not sure if it qualifies as a bug or not.

I recently switched all but two mutations over to this.props.relay.commitUpdate from Relay.Store.commitUpdate. My login form didn't have any Relay fragments, so it wasn't necessary to make it a relay container...so it didn't have this.props.relay and was still using Relay.Store.commitUpdate. This was not hitting the URL network layer middleware. Making that form a Relay container and using this.props.relay.commitUpdate fixed it so that it does pass through the URL middleware.

Is that expected behavior?

nodkz commented 7 years ago

@rojobuffalo suppose that you have two Relay.Store (environments). One with Relay-network-layer, the second with default network layer. That's why you may receive such behavior.

ryanblakeley commented 7 years ago

@nodkz that makes sense. :+1: