Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom Request Pre-Processing #9

Closed
thomasloh opened this issue Dec 18, 2015 · 11 comments
Closed

Custom Request Pre-Processing #9

thomasloh opened this issue Dec 18, 2015 · 11 comments

Comments

@thomasloh
Copy link

for example, ability to specify default header(s) to be added to all requests

@ryanbrainard
Copy link
Contributor

I have considered something like this, and not a bad idea, but so far, I've done things like this, which has worked in the limited places I have needed it:

export default connect(({ params: { userId }, location: pathname }) => {
  const common = (urls) => Object.keys(urls).reduce((reqs, p) => {
    reqs[p] = {
      url: urls[p],
      comparison: pathname,
      refreshing: true,
      headers: {
        'X-Custom-Header': 'true'
      }
    }
    return reqs
  }, {})

  return common({
    userFetch: `/proxy/users/${userId}`,
    likesFetch:  `/proxy/likes/${userId}`,
  })
})(Profile)

If we did something like this, I'm thinking some kind of similar function set in options that would be run on every request after it has been coerced into a canonical request (i.e. in the full object form, not just a URL), but before fetch() is called. Is that what you were thinking?

@thomasloh
Copy link
Author

@ryanbrainard yeah exactly. A way to specify common metadata to be coerced into the final canonical request, and should allow overrides as well.

The bigger question is probably how/where to do it, because the metadata should only be specified in one place, upfront somewhere. And then is used by all connect(), or selectively via decorators

@ryanbrainard
Copy link
Contributor

@thomasloh I'm hesitant to add any global options, but this could be a common option that's passed into every connect(). It would be just one extra line, so not too much boilerplate and would avoid global/singleton messiness. Something like this:

export default connect(({ params: { userId }, location: pathname }) => {
  return {
    userFetch: `/proxy/users/${userId}`,
    likesFetch:  `/proxy/likes/${userId}`,
  }
}, requestPreprocessor: myCommonRequestPreprocessor)(Profile)

where myCommonRequestPreprocessor is a function similar to the common() one I showed before but defined in a separate file so it can be shared. I'm thinking the function would probably take (requests, props) and return requests. How does that sound? Also, open to a better, shorter name than requestPreprocessor.

Another way to possibly do it is instead of making it a pre-processor, it would be letting applications overrides the defaults used in buildRequest().

Also to consider: the order of exactly when this happens. Should it be before, after, or completely replace buildRequest()?

@thomasloh
Copy link
Author

@ryanbrainard oh yeah no definitely don't want to maintain any state there! I should rephrase, it should be defined once somewhere, like you suggested, in a separate module/file, which can then be used by interested connect().

Personally I like the idea of preprocessors, because it behaves like middleware and allow composition. Something like,

// requestPreprocessor.js

const compose = (...fns) => (reqs, props) => fns.reduce((finalReqs, fn) =>  fn(finalReqs, props), reqs);

function addAuthorizationHeader(reqs, props) {
}

function addComparison(reqs, props) {
}

export default compose(
  addAuthorizationHeader,
  addComparison
  ...
)

Calling it just preprocessor sounds good to me, so like

import refetchPreprocessor from "/common/refetch-preprocessor" // naming of this is in userland of course

connect(() => {...}, preprocessor: refetchPreprocessor)

@hnordt
Copy link
Contributor

hnordt commented Dec 21, 2015

@ryanbrainard

I liked your approach, maybe we can support something like enhancers.

I think it's like the middleware idea from Redux.

You provide the common usage, but people can create enhancers.

For example, you don't need to handle CSV requests, you can create a enhancer for it. You provide the core, people provide "uncommon" things using enhancers.

Usage

import myRequestPreprocessor from './myRequestPreprocessor.js';
import handleCSV from 'refetch-csv';
import { handleJWTRequest, handleJWTResponse } from 'refetch-jwt';

// if you have many enhancers, and want to use it on many components,
// just create a `request-enhancers.js` and/or `response-enhancers.js` files and 
// export your enhancers as an array

// second param: request enhancers (accept one enhancer or an array of enhancers)
// third param: response enhancers (accept one enhancer or an array of enhancers)
export default connect(props => ({ foo: 'bar' }), [myRequestPreprocessor, handleJWTRequest], [handleCSV, handleJWTResponse]);

myRequestPreprocessor.js

// the returned object is merged into react-refetcht's default request
const requestPreprocessor = request => ({
  url: `http://foo.com/api/${request.url}`,
  headers: {
    'foo': 'bar'
  }
});

export default requestPreprocessor;

refetch-csv

// the resolved promise should be the final response
const handleCSV = (request, response) => {
  return Promise((resolve, reject) => {
    if (request.dataType === 'csv') {
      myCSVParser(response.body)
        .then(data => resolve(Object.assign({}, response, { body: data })))
        .catch(error => reject(new Error(error));
    }
    resolve(response);
  });
};

export default handleCSV;

refetch-jwt

// if you return nothing, nothing is merged
const handleJWTRequest = request => {
  if (localStorage.token) {
    return {
      headers: { Authorization: `Token: ${localStorage.token}` }
    }
  }
};

// of course if we release it as a npm module, we should support a configurable pathname
const handleJWTResponse = (request, response) => {
  return Promise((resolve, reject) => {
    if (request.pathname === '/auth/login' && response.ok) {
      localStorage.token = response.body.token;
    }
    const UNAUTHORIZED = 401;
    if ((request.pathname === '/auth/logout' && response.ok) || response.status === UNAUTHORIZED) {
      localStorage.token = null;
    }
    resolve(response);
  });
};

export default { handleJWTRequest, handleJWTResponse };

@ryanbrainard ryanbrainard changed the title Support pre-defined request metadata Custom Request Pre-Processing Jan 4, 2016
@LucaColonnello
Copy link

@hnordt Great Idea!! I'm waiting this ti use this lib in production! Are you making some of these changes?

@apsaros
Copy link

apsaros commented Jan 17, 2016

@hnordt +1
and great work with the lib @ryanbrainard :)

@hnordt
Copy link
Contributor

hnordt commented Jan 19, 2016

@ryanbrainard my suggestion on #40 (comment) can handle this.

@eyalw
Copy link
Contributor

eyalw commented Feb 17, 2016

May I suggest fetch-intercept?
It allows both interception of request and response, globally in the app,
great for response value adapters, and request header settings (like csrf).

@ryanbrainard
Copy link
Contributor

v1.0.0-beta.0 has the ability to change the buildRequest, fetch, and handle Response implementations, which should open up a lot of possibilities here. These features are probably a little too low level for everyday pre-processing; however, I want people to kick the tires on them and see what can possibly be built on top of them. I'm going to close this issue now so that we can re-frame remaining enhancements in this new context by opening new issues.

@LucaColonnello
Copy link

Good work guys!

Il lunedì 28 marzo 2016, Ryan Brainard notifications@github.com ha
scritto:

v1.0.0-beta.0
https://github.com/heroku/react-refetch/releases/tag/v1.0.0-beta.0 has
the ability to change the buildRequest, fetch, and handle Response
implementations, which should open up a lot of possibilities here. These
features are probably a little too low level for everyday pre-processing;
however, I want people to kick the tires on them and see what can possibly
be built on top of them. I'm going to close this issue now so that we can
re-frame remaining enhancements in this new context by opening new issues.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#9 (comment)

Luca Colonnello
+39 345 8948718
luca.colonnello91@gmail.com

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants