Skip to content

Commit

Permalink
Merge interceptors
Browse files Browse the repository at this point in the history
  • Loading branch information
@drewjbartlett
drewjbartlett committed Apr 5, 2018
1 parent 0428632 commit 1976a76
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 0 deletions.
28 changes: 28 additions & 0 deletions dist/core/core.js
View file
Expand Up @@ -126,6 +126,7 @@ var Core = function () {
key: 'initializeAPI',
value: function initializeAPI() {
this.api = _axios2.default.create((0, _defaultsDeep2.default)({ baseURL: this.config.baseURL.replace(/\/$/, '') }, this.config.apiConfig));
this.writeInterceptorsToAPI();
}

/**
Expand Down Expand Up @@ -157,6 +158,28 @@ var Core = function () {
}
}

/**
* Set the interceptors to the api object
*/

}, {
key: 'writeInterceptorsToAPI',
value: function writeInterceptorsToAPI() {
var _this3 = this;

var interceptors = this.config.interceptors;

var types = Object.keys(interceptors);

if (types.length) {
types.forEach(function (type) {
interceptors[type].forEach(function (interceptor) {
_this3.api.interceptors[type].use(interceptor);
});
});
}
}

/**
* Resets the request data
*/
Expand Down Expand Up @@ -202,6 +225,11 @@ var Core = function () {

return this;
}
}, {
key: 'interceptors',
get: function get() {
return this.config.interceptors;
}
}, {
key: 'baseURL',
set: function set(url) {
Expand Down
11 changes: 11 additions & 0 deletions dist/core/defaults.js
View file
Expand Up @@ -67,6 +67,17 @@ exports.default = {
*/
globalParameters: {},

/**
* An optional collection of interceptors for requests and responses.
*
* @type {Object}
* @see https://github.com/axios/axios#interceptors
*/
interceptors: {
request: [],
response: []
},

/**
* The default request methods for the CRUD methods.
*
Expand Down

6 comments on commit 1976a76

@M3psipax
Copy link

@M3psipax M3psipax commented on 1976a76 Apr 12, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interceptors seem to work slightly differently than in axios. Specifically, I can't get my response error interceptor to work.
On axios I did:

// Add a response interceptor
axios.interceptors.response.use(response => {
    // Do something with response data
    return response;
  }, error => {
    // Do something with response error
    return Promise.reject(error);
  });

which added a response and an error interceptor.
Doing this with rapidjs like so doesn't seem to work:

this.interceptors.response.push(response => {
//something
}, error => {
// something with error
}

The response interceptor is called for valid responses. However, it's not called when an error occurs. How am I supposed to add an error interceptor? I get that there's no documentation on it since it's a very recent change. Can you provide some insight on how to correctly define the interceptors?

@drewjbartlett
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mgred would you mind responding here?

@M3psipax
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I took a deep look at it and here's where the error is. When adding interceptors using plain axios like this:

// Add a response interceptor
axios.interceptors.response.use(response => {
    // Do something with response data
    return response;
  }, error => {
    // Do something with response error
    return Promise.reject(error);
  });

you are calling a specific funtion that puts the two arguments into a specific 'handler' array as you can see here in line 17:


/**
 * Add a new interceptor to the stack
 *
 * @param {Function} fulfilled The function to handle `then` for a `Promise`
 * @param {Function} rejected The function to handle `reject` for a `Promise`
 *
 * @return {Number} An ID used to remove interceptor later
 */
InterceptorManager.prototype.use = function use(fulfilled, rejected) {
  this.handlers.push({
    fulfilled: fulfilled,
    rejected: rejected
  });
  return this.handlers.length - 1;
};

with the implementation you currently have in rapid.js, a reject handler cannot be added because here's what happens if you try:
the interceptors are just stored as a plain array. so for example if you want to put a response interceptor, you do:


this.interceptors.response.push(response => {
//something
})

this works fine for normal responses, but you have no way to add a handler for the response rejection, since you're just pushing functions onto the array.

this array is then used in the API initializiation like this:

  /**
   * Set the interceptors to the api object
   */
  writeInterceptorsToAPI () {
    const { interceptors } = this.config;
    const types = Object.keys(interceptors);

    if (types.length) {
      types.forEach((type) => {
        interceptors[type].forEach((interceptor) => {
          this.api.interceptors[type].use(interceptor);
        });
      });
    }
}

see here (line 106)

the 'types' are just "request" and "response".

so: types.forEach((type) => { will do its body for each function inside the 'this.interceptors.response' array.
so the body this.api.interceptors[type].use(interceptor); is always called with just a single argument, which is the funtion in the rapid's response interceptor array. this means you can never add a 'reject' argument.

rapid.js should provide an interface that is similar to axios. maybe, it should have two 'InterceptorManager' Objects from axios named 'interceptors.request' and 'interceptors.response' instead of plain arrays.

Then in method 'writeInterceptorsToAPI' it should do:

  writeInterceptorsToAPI () {
    const { interceptors } = this.config;
    const types = Object.keys(interceptors);

    if (types.length) {
      types.forEach((type) => {
        interceptors[type].forEach((interceptorManager) => {
          // either pass two arguments
          this.api.interceptors[type].use(interceptorManager.handlers[0], interceptorManager.handlers[1]);
          // or if its' possible, just assign the manager
          this.api.interceptors[type] = interceptorManager
        });
      });
    }
}

@mgred
Copy link
Collaborator

@mgred mgred commented on 1976a76 Apr 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@M3psipax Thanks for your very detailed report!

rapid.js should provide an interface that is similar to axios. maybe, it should have two 'InterceptorManager' Objects from axios named 'interceptors.request' and 'interceptors.response' instead of plain arrays.

I totally agree here. I will open a new issue and change this regarding your proposals.

@drewjbartlett fyi

@M3psipax
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, awesome, thanks!

fyi my current workaround is to attach the interceptors to the underlying axios instance like this:

class Base extends Rapid{
.....
  initializeAPI() {
    super.initializeAPI();
    this.api.interceptors.response.use(response => {
      //response interception
     return response;
    }, error => {
      // error interception
      return Promise.reject(error.response);
    });
  }
}

@mgred
Copy link
Collaborator

@mgred mgred commented on 1976a76 Apr 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks for sharing!
#43 is the related issue.

Please sign in to comment.