Skip to content
This repository was archived by the owner on Feb 20, 2025. It is now read-only.

Comments

Added ability to specify a default url in the config. #29

Open
bobjohnbob wants to merge 1 commit intoradiosilence:masterfrom
bobjohnbob:master
Open

Added ability to specify a default url in the config. #29
bobjohnbob wants to merge 1 commit intoradiosilence:masterfrom
bobjohnbob:master

Conversation

@bobjohnbob
Copy link

This pull request allows users to set the url with the config. To accommodate this, the first parameter to a GET/POST/etc request is now instead considered a subPath to that url.

If no url is configured, the subPath is treated as a url.

Also, if the subPath begins with {protocol}:// the configured url is ignored.

This should prevent breaking the old API and allow the developer to specify a default protocol://domain:port/basePath to use on all requests. This is particularly useful if you're making a bunch of calls to an api across domains.

…d are instead subpaths unless they start with protocol://
@bobjohnbob
Copy link
Author

This is my first pull request. Constructive criticism encouraged, but please be gentle ;)

function getURI(opts) {
let uri = opts.url;
let subPath = opts.subPath
if(!uri|| subPath.indexOf("://") !== -1) {

Choose a reason for hiding this comment

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

A substantial number of users are likely to use 'protocol relative' urls - which will not contain ://
This probably won't break existing implementations (since folks are unlikely to have specified a default url as the base url) - however it will prevent the usage of the override should a base url be specified during initialization.

@bobjohnbob
Copy link
Author

Thanks for taking the time to review my changes and provide valuable input, @masterful. If @radiosilence decides this is a feature he would like to merge in, I am willing to put in the work to fix the code to address these comments. I will probably do it anyways in my own fork, when I have time :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants