Amp subscriptions: Read service config from page #13611
Conversation
prateekbh
commented
Feb 22, 2018
prateekbh
commented
- Reads service config from page
- Waits for service config before initializing
- checks for authorization Url in service config
…mp-subscriptions-sconfig
…kbh/amphtml into amp-subscriptions-sconfig
|
@dvoytenko Looks like renaming changes did not got committed in the last PR, so they are here in this one |
examples/amp-subscriptions.amp.html
Outdated
| { | ||
| "services": [ | ||
| { | ||
| "serviceId":"amp.local.subscription", |
There was a problem hiding this comment.
Let's remove serviceId here and just default to "local".
examples/amp-subscriptions.amp.html
Outdated
| "authorizationUrl": "/subscription/2/entitlements" | ||
| }, | ||
| { | ||
| "serviceId":"google.subscription" |
| /** @private @const {!Array<!SubscriptionPlatform>} */ | ||
| this.subscriptionPlatforms_ = []; | ||
|
|
||
| /** @private {?EntitlementStore} */ | ||
| this.entitlementStore_ = null; | ||
|
|
||
| /** @const @private {!Element} */ | ||
| this.configElement_ = dev().assertElement(configElement); |
There was a problem hiding this comment.
Let's use user().assertElement here
| ]); | ||
|
|
||
| return configPromises.then(promiseValues => { | ||
| const serviceConfig = user().assert(promiseValues[0]); |
There was a problem hiding this comment.
You have to add typing for serviceConfig and pageConfig.
There was a problem hiding this comment.
Like a /** typedef / or a just /* {!PageConfig} ? and /* JsonObject<string, string> */
There was a problem hiding this comment.
/** @type {!PageConfig} */
| * @private | ||
| */ | ||
| initializeSubscriptionPlatforms_(serviceConfig, pageConfig) { | ||
| if (serviceConfig['authorizationUrl']) { |
There was a problem hiding this comment.
This should be that if ((serviceConfig['serviceId'] || 'local') == 'local') {...}
| @@ -51,12 +51,13 @@ export class LocalSubscriptionPlatform { | |||
| * @return {!Promise<!Entitlements>} | |||
| */ | |||
| getEntitlements() { | |||
| user().assert(this.serviceConfig_['paywallUrl'], | |||
| 'Service config does not have paywall Url'); | |||
| const authUrl = user().assert(this.serviceConfig_['authorizationUrl'], | |||
There was a problem hiding this comment.
It's best to initialize all authorizationUrl in the constructor and also call assertHttpsUrl on it.
| return this.xhr_ | ||
| .fetchJson(this.serviceConfig_['paywallUrl']) | ||
| .fetchJson(authUrl) |
There was a problem hiding this comment.
We need to make sure all CORS stuff is added here.
There was a problem hiding this comment.
Is there something like a util for this?
There was a problem hiding this comment.
It's the fetchJson API. I believe it's something like this:
fetchJson(url, {
credentials: 'include',
})
But please double-check.
| return this.xhr_ | ||
| .fetchJson(this.serviceConfig_['paywallUrl']) | ||
| .fetchJson(this.authorizationUrl_,{ |
There was a problem hiding this comment.
nit: space between "," and "{"
| return this.xhr_ | ||
| .fetchJson(this.serviceConfig_['paywallUrl']) | ||
| .fetchJson(this.authorizationUrl_,{ | ||
| mode: 'cors', |
There was a problem hiding this comment.
No need for mode here. Found this: https://github.com/ampproject/amphtml/blob/master/extensions/amp-access/0.1/amp-access-client.js#L126
| .fetchJson(this.serviceConfig_['paywallUrl']) | ||
| .fetchJson(this.authorizationUrl_,{ | ||
| mode: 'cors', | ||
| credentials: 'include', |
* fix animating circle fixed position * adding null resolve if no entitlements enables reader * adding tests * adding tests for entitlements store * lint fixes * sending page config to subscription platform * fixing comments * changing promise resolution of firstPositive * reading serviceConfig * fixing existing tests * adding test for service config * fixed nits * fixing conflicts * fixing comments * fixing cors and types * adding more test
