josx / ra-data-feathers

A feathers rest client for react-admin
MIT License
157 stars 53 forks source link

AuthClient: check if not any type #60

Closed josx closed 6 years ago

josx commented 6 years ago

Right now if not any of react-admin types would throw an error, but in the docs tells to reject promise.

Is it the same?

https://github.com/josx/aor-feathers-client/blob/master/src/authClient.js#L74

nicholasnelson commented 6 years ago

Throwing errors and returning Promise.reject seem to be handled the same by Redux Saga (which is used in React Admin). The anonymous function in authClient.js is called by Redux Saga's runCallEffect:

  function runCallEffect(_ref4, effectId, cb) {
    var context = _ref4.context,
        fn = _ref4.fn,
        args = _ref4.args;

    var result = void 0;
    // catch synchronous failures; see #152
    try {
      result = fn.apply(context, args);
    } catch (error) {
      return cb(error, true);
    }
    return is.promise(result) ? resolvePromise(result, cb) : is.iterator(result) ? resolveIterator(result, effectId, fn.name, cb) : cb(result);
  }

If the function throws an error, it's caught and the result passed as a param to cb(error, true).

If the function returns a promise (which rejects), calls resolvePromise(result, cb) which does this:

    promise.then(cb, function (error) {
      return cb(error, true);
    });

tl;dr I don't think it makes any difference whether you throw an error or return Promise.reject as Redux Saga does some Magic behind the scenes to ensure both options give the same result. It might be best to use Promise.reject purely from a consistency standpoint (both with the documentation for React Admin and the other uses of Promise.reject over throw in this file).

josx commented 6 years ago

thanks @nicholasnelson , i think we need to keep consistency.

So would be welcomed a PR replacing that throw error with a promise.reject

nicholasnelson commented 6 years ago

See above PR. Made the same change in restClient as well.

josx commented 6 years ago

great! merged!