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

Optional urlPrefix and apiVersion for Platform.createUrl. #95

Merged
merged 7 commits into from
Feb 14, 2018

Conversation

MostFrumiousBandersnatch
Copy link
Contributor

This is how we're about to utilize it:

 sdk().platform().createUrl(
       this.getRawURI(args),
       {
             addToken: true,
             addServer: true
             urlPrefix: '/rcvideo',
             apiVersion: 'v1'
       }
)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 91.285% when pulling 7da77f0 on MostFrumiousBandersnatch:master into 23c96e0 on ringcentral:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 91.285% when pulling 7da77f0 on MostFrumiousBandersnatch:master into 23c96e0 on ringcentral:master.

@coveralls
Copy link

coveralls commented Feb 8, 2018

Coverage Status

Coverage increased (+0.05%) to 91.3% when pulling ad08cdf on MostFrumiousBandersnatch:master into 23c96e0 on ringcentral:master.

@kirill-konshin
Copy link
Contributor

The better idea would be to just add something like knownPrefixes property because otherwise you will have to manually run createUrl for any request that you send. Not convenient...

@MostFrumiousBandersnatch
Copy link
Contributor Author

Not sure, that it would a better idea.

Thus, we would initialize the SDK object with some knownPrefixes and, then call createUrl pointing one of them? How? Implicitly, by number? Either explicitly, and face with possible rejections when given values aren't ones among predefined knownPrefixes?
That's a responsibility of higher-level code, to provide a necessary abstraction around createUrl. The SDK itself hardly should be concerned about that.
Nothing would prevent us from wrapping createUrl into few different functions, specifying different options in each one.

@kirill-konshin
Copy link
Contributor

The idea is that if createUrl that is called by get|post|put|delete (with no params) will see the prefix it will not do anything with the URL. We can add knownPrefixes array in Platform class AND an escape latch in SDK constructor to override/append to that array if needed.

I don't think there will be too many prefixes and that they will appear very frequently.

We should not force customers to figure out what function to call to format URL correctly: in my opinion, platform.get(platform.createUrl(...), {things}) is not very user friendly.

@MostFrumiousBandersnatch
Copy link
Contributor Author

I decided to neglect the merging predefined knownPrefixes with passed knownPrefixes, in order to keep code base small (avoiding extra dependencies). Let me know if it's (necessity of merge) really a case.


if (options.addServer && !hasHttp) builtUrl += this._server;

if (path.indexOf(Platform._urlPrefix) == -1 && !hasHttp) builtUrl += Platform._urlPrefix + '/' + Platform._apiVersion;
if (path.indexOf(Platform._urlPrefix) == -1 && !hasHttp && !alreadyPrefixed) {
builtUrl += [Platform._urlPrefix, Platform._apiVersion].join('/');
Copy link
Contributor

Choose a reason for hiding this comment

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

Weird code style to add a slash between two strings )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just more efficient than string concatenation. Anyway, i can replace it.

@@ -78,6 +78,10 @@ function Platform(options) {
/** @private */
this._client = options.client;

/** @private */
this._knownPrefixes = options.knownPrefixes || Platform._knownPrefixes;
Copy link
Contributor

Choose a reason for hiding this comment

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

That's OK.

@kirill-konshin
Copy link
Contributor

All good. Please bump minor version, make a build and update this PR. Also please figure out what's wrong with failed CI on Node 4, maybe we should kick it out of stack.

@MostFrumiousBandersnatch
Copy link
Contributor Author

I have no idea why it fails just on Node 4, being run in Travis. Error seems to be irrelevant to node version, furthermore it run normally on my machine, using the very same version of node.
Anyway brief investigation of the issue reveals something:

Subscription-spec.js

return subscription.setEventFilters(['foo', 'bar'])

Subscription.js

Subscription.prototype._getFullEventFilters = function() {
    return this.eventFilters().map(function(event) {
        return this._platform.createUrl(event);
    }.bind(this));
}

which generates suspicious URLs:

> sdk.platform().createUrl('foo');
'/restapi/v1.0foo

Hope, that may give you a hint to fix that issue.

@kirill-konshin
Copy link
Contributor

Remove Node 4 from travis.yml

@MostFrumiousBandersnatch
Copy link
Contributor Author

Are you sure to remove it? Version of node is hardly a reason, assuming results of recent build .

@kirill-konshin kirill-konshin merged commit 617a386 into ringcentral:master Feb 14, 2018
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.

None yet

3 participants