search

feedhenry / fh-js-sdk

FeedHenry Javascript SDK
http://feedhenry.org/fh-js-sdk/
Apache License 2.0
8 stars 37 forks source link

Remove FeedHenry Params in Query/Body #149

Closed evanshortiss closed 8 years ago

evanshortiss commented 8 years ago

Hey,

Currently when making a FH.Cloud call the result of FH.getCloudParams is injected to the body/query. This is a highly intrusive behaviour that would be unwanted by people building an API in express since they have to deal with removing these from a query/body.

Placing these in the headers would be much cleaner and it appears we do that. I'll take a look to see why this is not being added to my FH.Cloud call headers, but would we be open to removing or adding a config option to prevent the FH.getCloudParams from being injected into user queries/bodies?

@wei-lee

evanshortiss commented 8 years ago

I see the headers are being added successfully in my app. So my next question is can we consider removing the addition of params to the query/body since the headers cover it?

If not, then can we expose a method/option to enable developers to disable the injection of the params to a query/body?

evanshortiss commented 8 years ago

Made a mistake, I mean FH.getFHParams in above comments

compjuicer commented 8 years ago

+1, especially because the same attributes are in the HTTP headers already

wei-lee commented 8 years ago

@evanshortiss @compjuicer I think that data in the request body was a legacy thing. It's for work around some of the problems we have before:

  1. IE 8 and windows phone 7 doesn't support ajax calls with headers
  2. There was a bug where our cloud app doesn't allow arbitrary headers in the CORS requests

Not sure if we can drop IE8 support just yet, but we certain can just send the data in the body for IE 8 only.

Issue 2 shouldn't be a problem anymore. All the cloud app should accept any headers in the CORS requests.

So I hope will fix this in next release - but probably will be in Jan next year.

evanshortiss commented 8 years ago

Awesome news. Thanks @wei-lee this will clean things up a lot for cloud requests.

On Thursday, December 10, 2015, Wei Li notifications@github.com wrote:

@evanshortiss https://github.com/evanshortiss @compjuicer https://github.com/compjuicer I think that data in the request body was a legacy thing. It's for work around some of the problems we have before:

  1. IE 8 and windows phone 7 doesn't support ajax calls with headers
  2. There was a bug where our cloud app doesn't allow arbitrary headers in the CORS requests

Not sure if we can drop IE8 support just yet, but we certain can just send the data in the body for IE 8 only.

Issue 2 shouldn't be a problem anymore. All the cloud app should accept any headers in the CORS requests.

So I hope will fix this in next release - but probably will be in Jan next year.

— Reply to this email directly or view it on GitHub https://github.com/feedhenry/fh-js-sdk/issues/149#issuecomment-163704359 .

evanshortiss commented 8 years ago

@wei-lee is this included now? Should we close this?

wei-lee commented 8 years ago

@evanshortiss I don't believe this is fixed yet, better leave it open.

evanshortiss commented 8 years ago

Thanks for the update @wei-lee!