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

Remove corsProxy or factor out as an option #2

Closed
maxsatula opened this issue Jan 7, 2018 · 7 comments
Closed

Remove corsProxy or factor out as an option #2

maxsatula opened this issue Jan 7, 2018 · 7 comments

Comments

@maxsatula
Copy link
Collaborator

https://cors.now.sh/ seems to be dead

I removed any references to corsProxy variable to make this module work.

Either remove it or make it a configurable option, in case if anyone still needs a proxy.

Thanks

@TooTallNate
Copy link
Owner

👍 Pull request welcome for a proxy option. Note though that we need a default proxy when in the browser only, since CORS headers are not present in the browser. In Node.js this isn't a problem, but for the sake of code simplicity I didn't fork the logic originally.

@maxsatula
Copy link
Collaborator Author

I do not want to produce bad code. Not sure how to do it better.
First option may look better, but will break existing code whoever uses the module (they will need to add extra parenthesis to require:

const iheart = require('iheart')(); // default proxy
const iheart = require('iheart')({ proxy: "http://blah-blah.com" }); // use specific non-default
const iheart = require('iheart')({ proxy: null }); // no proxy

Option number two:

const iheart = require('iheart'); // good old syntax here
iheart.setOptions({ proxy: "http://blah-blah.com" });

Or maybe option number 3 (no idea what).

@TooTallNate
Copy link
Owner

IMO we should add an Options parameter to each of the API functions and have proxy be the only option supported for now.

@TooTallNate
Copy link
Owner

So iheart.search('107.7', { proxy: 'foo' })

@maxsatula
Copy link
Collaborator Author

Looks equally bad. User will have to carry that option everywhere.

@TooTallNate
Copy link
Owner

Indeed, but you have to do the same thing with i.e. agent for http.Client requests in Node.js, so I think it's the most aligned with the Node API that way.

@TooTallNate
Copy link
Owner

TooTallNate commented Jan 7, 2018

Not totally relevant to the issue at hand, but https://cors.now.sh is back up

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

No branches or pull requests

2 participants