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

Fix serverSync to use apiProxy logic in the same way the clientSync does #256

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
18 changes: 11 additions & 7 deletions server/middleware/apiProxy.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,27 +13,31 @@ var separator = '/-/';
module.exports = apiProxy;

function apiProxy(dataAdapter) {
return function(req, res, next) {
var middleware = function(req, res, next) {
var api;

api = _.pick(req, 'query', 'method', 'body');

api.path = apiProxy.getApiPath(req.path);
api.api = apiProxy.getApiName(req.path);
api.headers = {
'x-forwarded-for': apiProxy.getXForwardedForHeader(req.headers, req.ip)
};

dataAdapter.request(req, api, {
convertErrorCode: false
}, function(err, response, body) {
middleware.proxyRequest(req, res, api, { convertErrorCode: false }, function(err, response, body) {
if (err) return next(err);

// Pass through statusCode.
res.status(response.statusCode);
res.json(body);
});
};

middleware.proxyRequest = function proxyRequest(req, res, api, options, callback) {
api.headers = api.headers || {};
api.headers['x-forwarded-for'] = apiProxy.getXForwardedForHeader(req.headers, req.ip);

dataAdapter.request(req, api, options, callback);
};

return middleware;
};

apiProxy.getApiPath = function getApiPath(path) {
Expand Down
5 changes: 5 additions & 0 deletions server/middleware/initApp.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ module.exports = function(appAttributes, options) {
* This will only be accessible on the server.
*/
req: req,
res: res,
Copy link
Member

Choose a reason for hiding this comment

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

This isn't strictly necessary because req and res have references to each other -- i.e. you could do this.app.req.res

entryPath: options.entryPath,
modelUtils: options.modelUtils
};
Expand All @@ -50,6 +51,10 @@ module.exports = function(appAttributes, options) {

var app = new App(attributes, appOptions);

if (options.proxyRequest) {
app.proxyRequest = options.proxyRequest;
}

/**
* Stash the app instance on the request so can be accessed in other middleware.
*/
Expand Down
30 changes: 8 additions & 22 deletions server/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,35 +80,22 @@ function Server(options) {
Server.prototype.configure = function(fn) {
var dataAdapter = this.dataAdapter,
apiPath = this.options.apiPath,
notApiRegExp = new RegExp('^(?!' + apiPath.replace('/', '\\/') + '\\/)');
notApiRegExp = new RegExp('^(?!' + apiPath.replace('/', '\\/') + '\\/)'),
apiProxyMiddleware;

this._configured = true;
this.options.apiProxy = this.options.apiProxy || middleware.apiProxy;
apiProxyMiddleware = this.options.apiProxy(dataAdapter);

/**
* Attach the `dataAdapter` to the `req` so that the `syncer` can access it.
*/
this.expressApp.use(function(req, res, next) {
req.dataAdapter = dataAdapter;

/**
* Proxy `res.end` so we can remove the reference to `dataAdapter` to prevent leaks.
*/
var end = res.end;
res.end = function(data, encoding) {
res.end = end;
req.dataAdapter = null;
res.end(data, encoding);
};
next();
});
this._configured = true;

/**
* Initialize the Rendr app, accessible at `req.rendrApp`.
*/
this.expressApp.use(middleware.initApp(this.options.appData, {
apiPath: this.options.apiPath,
entryPath: this.options.entryPath,
modelUtils: this.options.modelUtils
modelUtils: this.options.modelUtils,
proxyRequest: apiProxyMiddleware.proxyRequest
}));

/**
Expand All @@ -120,8 +107,7 @@ Server.prototype.configure = function(fn) {
/**
* Add the API handler.
*/
this.options.apiProxy = this.options.apiProxy || middleware.apiProxy;
this.expressApp.use(this.options.apiPath, this.options.apiProxy(dataAdapter));
this.expressApp.use(this.options.apiPath, apiProxyMiddleware);

/**
* Add the routes for everything defined in our routes file.
Expand Down
4 changes: 4 additions & 0 deletions shared/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ module.exports = Backbone.Model.extend({
this.req = this.options.req;
}

if (this.options.res) {
this.res = this.options.res;
}

/**
* Initialize the `templateAdapter`, allowing application developers to use whichever
* templating system they want.
Expand Down
5 changes: 3 additions & 2 deletions shared/syncer.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,14 +58,15 @@ function clientSync(method, model, options) {
}

function serverSync(method, model, options) {
var api, data, urlParts, verb, req;
var api, data, urlParts, verb, req, res;

data = _.clone(options.data);
data = addApiParams(method, model, data);
options.url = this.getUrl(options.url, false, data);
verb = methodMap[method];
urlParts = options.url.split('?');
req = this.app.req;
res = this.app.res;

api = {
method: verb,
Expand All @@ -85,7 +86,7 @@ function serverSync(method, model, options) {
_.extend(api.query, data);
}

req.dataAdapter.request(req, api, function(err, response, body) {
this.app.proxyRequest(req, res, api, {}, function(err, response, body) {
var resp;
if (err) {
resp = {
Expand Down