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

Conversation

lxanders
Copy link
Contributor

Formerly the serverSync method in syncer.js used the dataAdapter to request data from an api host. This is a problem as e.g. the x-forward-for header is only set in the apiProxy. The serverSync method should work the same way as the clientSync method which already uses the apiProxy.

We extracted the proxy request logic to a reusable method proxyRequest stored in the apiProxyMiddleware. Additionally we added the res object to the global app object in the same way the req object is already being added to it, to allow a custom apiProxy making use of the res object.

One additional advantage would be the decoupling of the dataAdapter from the syncer which e.g. makes it simple to implement support for multiple dataAdapters (e.g. for REST and for SOAP).

CC: @c089, @selaux, @lo1tuma

@@ -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

@spikebrehm
Copy link
Member

Thanks for pointing this out. I think it might make more sense to just move the X-Forwarded-For logic directly to the DataAdapter; it shouldn't really live on the middleware. That would simplify this a bit, yeah? What do you think?

@c089
Copy link
Member

c089 commented Jan 13, 2014

I'm with Spike on this one: We've been mixing responsibilities in the apiProxy a bit, which should only be concerned about proxying requests from the browser to the API.

I'm not sure if moving to DataAdapter is the best way though. We seem to be missing an extension point that would work for all requests (regardless of the environment they originated from) but is no in the DataAdapter. As I think in the long term we want to support separate DataAdapters (configurable per API, there's a ticket somewhere I think) -> that would require adding things like the x-forwarded-for thing to all of them...

@lxanders
Copy link
Contributor Author

After discussing with c089 I'm with you now. The problem itself is still valid though so I'm not closing the pull request for now (it would have been a good idea to describe the problem in an issue and have the discussion there - next time).

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

Successfully merging this pull request may close these issues.

3 participants