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

Tighten types related to element attribute modification. #10091

Merged
merged 1 commit into from Jun 22, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 4 additions & 3 deletions builtins/amp-pixel.js
Expand Up @@ -16,6 +16,7 @@

import {BaseElement} from '../src/base-element';
import {dev, user} from '../src/log';
import {dict} from '../src/utils/object';
import {registerElement} from '../src/custom-element';
import {timerFor} from '../src/services';
import {urlReplacementsForDoc} from '../src/services';
Expand Down Expand Up @@ -119,9 +120,9 @@ function createNoReferrerPixel(parentElement, src) {
// if "referrerPolicy" is not supported, use iframe wrapper
// to scrub the referrer.
const iframe = createElementWithAttributes(
/** @type {!Document} */ (parentElement.ownerDocument), 'iframe', {
src: 'about:blank',
});
/** @type {!Document} */ (parentElement.ownerDocument), 'iframe', dict({
'src': 'about:blank',
}));
parentElement.appendChild(iframe);
createImagePixel(iframe.contentWindow, src);
return iframe;
Expand Down
57 changes: 30 additions & 27 deletions extensions/amp-a4a/0.1/amp-a4a.js
Expand Up @@ -37,6 +37,7 @@ import {
import {isLayoutSizeDefined} from '../../../src/layout';
import {isAdPositionAllowed} from '../../../src/ad-helper';
import {dev, user, duplicateErrorIfNecessary} from '../../../src/log';
import {dict} from '../../../src/utils/object';
import {getMode} from '../../../src/mode';
import {isArray, isObject, isEnumValue} from '../../../src/types';
import {some} from '../../../src/utils/promise';
Expand Down Expand Up @@ -97,14 +98,14 @@ export const XORIGIN_MODE = {
};

/** @type {!Object} @private */
const SHARED_IFRAME_PROPERTIES = {
frameborder: '0',
allowfullscreen: '',
allowtransparency: '',
scrolling: 'no',
marginwidth: '0',
marginheight: '0',
};
const SHARED_IFRAME_PROPERTIES = dict({
'frameborder': '0',
'allowfullscreen': '',
'allowtransparency': '',
'scrolling': 'no',
'marginwidth': '0',
'marginheight': '0',
});

/** @typedef {{
* creative: ArrayBuffer,
Expand Down Expand Up @@ -1269,16 +1270,17 @@ export class AmpA4A extends AMP.BaseElement {
// Create and setup friendly iframe.
this.iframe = /** @type {!HTMLIFrameElement} */(
createElementWithAttributes(
/** @type {!Document} */(this.element.ownerDocument), 'iframe', {
/** @type {!Document} */(this.element.ownerDocument), 'iframe',
dict({
// NOTE: It is possible for either width or height to be 'auto',
// a non-numeric value.
height: this.creativeSize_.height,
width: this.creativeSize_.width,
frameborder: '0',
allowfullscreen: '',
allowtransparency: '',
scrolling: 'no',
}));
'height': this.creativeSize_.height,
'width': this.creativeSize_.width,
'frameborder': '0',
'allowfullscreen': '',
'allowtransparency': '',
'scrolling': 'no',
})));
this.applyFillContent(this.iframe);
const fontsArray = [];
if (creativeMetaData.customStylesheets) {
Expand Down Expand Up @@ -1343,22 +1345,23 @@ export class AmpA4A extends AMP.BaseElement {

/**
* Shared functionality for cross-domain iframe-based rendering methods.
* @param {!Object<string, string>} attributes The attributes of the iframe.
* @param {!JsonObject<string, string>} attributes The attributes of the iframe.
* @return {!Promise} awaiting load event for ad frame
* @private
*/
iframeRenderHelper_(attributes) {
const mergedAttributes = Object.assign(attributes, {
height: this.creativeSize_.height,
width: this.creativeSize_.width,
});
const mergedAttributes = Object.assign(attributes, dict({
'height': this.creativeSize_.height,
'width': this.creativeSize_.width,
}));

if (this.sentinel) {
mergedAttributes['data-amp-3p-sentinel'] = this.sentinel;
}
this.iframe = createElementWithAttributes(
/** @type {!Document} */ (this.element.ownerDocument),
'iframe', Object.assign(mergedAttributes, SHARED_IFRAME_PROPERTIES));
'iframe', /** @type {!JsonObject} */ (
Object.assign(mergedAttributes, SHARED_IFRAME_PROPERTIES)));
// TODO(keithwrightbos): noContentCallback?
this.xOriginIframeHandler_ = new AMP.AmpAdXOriginIframeHandler(this);
// Iframe is appended to element as part of xorigin frame handler init.
Expand Down Expand Up @@ -1390,11 +1393,11 @@ export class AmpA4A extends AMP.BaseElement {
*/
renderViaCachedContentIframe_(adUrl) {
this.protectedEmitLifecycleEvent_('renderCrossDomainStart');
return this.iframeRenderHelper_({
src: xhrFor(this.win).getCorsUrl(this.win, adUrl),
name: JSON.stringify(
return this.iframeRenderHelper_(dict({
'src': xhrFor(this.win).getCorsUrl(this.win, adUrl),
'name': JSON.stringify(
getContextMetadata(this.win, this.element, this.sentinel)),
});
}));
}

/**
Expand Down Expand Up @@ -1446,7 +1449,7 @@ export class AmpA4A extends AMP.BaseElement {
name = `${this.safeframeVersion_};${creative.length};${creative}` +
`${contextMetadata}`;
}
return this.iframeRenderHelper_({src: srcPath, name});
return this.iframeRenderHelper_(dict({'src': srcPath, name}));
});
}

Expand Down
7 changes: 4 additions & 3 deletions extensions/amp-auto-ads/0.1/ad-network-config.js
Expand Up @@ -19,6 +19,7 @@ import {
getAdSenseAmpAutoAdsExpBranch,
} from '../../../ads/google/adsense-amp-auto-ads';
import {buildUrl} from '../../../ads/google/a4a/url-builder';
import {dict} from '../../../src/utils/object';
import {documentInfoForDoc} from '../../../src/services';
import {parseUrl} from '../../../src/url';
import {viewportForDoc} from '../../../src/services';
Expand Down Expand Up @@ -46,7 +47,7 @@ class AdNetworkConfigDef {
/**
* Any attributes derived from either the page or the auto-amp-ads tag that
* should be applied to any ads inserted.
* @return {!Object<string, string>}
* @return {!JsonObject<string, string>}
*/
getAttributes() {}

Expand Down Expand Up @@ -102,10 +103,10 @@ class AdSenseNetworkConfig {

/** @override */
getAttributes() {
return {
return dict({
'type': 'adsense',
'data-ad-client': this.autoAmpAdsElement_.getAttribute('data-ad-client'),
};
});
}

/** @override */
Expand Down
5 changes: 3 additions & 2 deletions extensions/amp-auto-ads/0.1/ad-strategy.js
Expand Up @@ -32,14 +32,15 @@ export class AdStrategy {

/**
* @param {!Array<!./placement.Placement>} placements
* @param {!Object<string, string>} baseAttributes Any attributes that should
* be added to any inserted ads. These will be combined with any
* @param {!JsonObject<string, string>} baseAttributes Any attributes that
* should be added to any inserted ads. These will be combined with any
* additional data atrributes specified by the placement.
* @param {!./ad-tracker.AdTracker} adTracker
*/
constructor(placements, baseAttributes, adTracker) {
this.availablePlacements_ = placements.slice(0);

/** @private {!JsonObject<string, string>} */
this.baseAttributes_ = baseAttributes;

/** @type {!./ad-tracker.AdTracker} */
Expand Down
5 changes: 3 additions & 2 deletions extensions/amp-auto-ads/0.1/amp-auto-ads.js
Expand Up @@ -53,8 +53,9 @@ export class AmpAutoAds extends AMP.BaseElement {
}

const placements = getPlacementsFromConfigObj(this.win, configObj);
const attributes = Object.assign(adNetwork.getAttributes(),
getAttributesFromConfigObj(configObj));
const attributes = /** @type {!JsonObject} */ (
Object.assign(adNetwork.getAttributes(),
getAttributesFromConfigObj(configObj)));
const adTracker =
new AdTracker(getExistingAds(this.win), adNetwork.getAdConstraints());
new AdStrategy(placements, attributes, adTracker).run();
Expand Down
18 changes: 10 additions & 8 deletions extensions/amp-auto-ads/0.1/anchor-ad-strategy.js
Expand Up @@ -15,6 +15,7 @@
*/
import {createElementWithAttributes} from '../../../src/dom';
import {user} from '../../../src/log';
import {dict} from '../../../src/utils/object';
import {viewportForDoc} from '../../../src/services';

/** @const */
Expand All @@ -26,15 +27,15 @@ const OPT_IN_STATUS_ANCHOR_ADS = 2;
export class AnchorAdStrategy {
/**
* @param {!Window} win
* @param {!Object<string, string>} baseAttributes Any attributes that should
* be added to any inserted ads.
* @param {!JsonObject<string, string>} baseAttributes Any attributes that
* should be added to any inserted ads.
* @param {!JSONType} configObj
*/
constructor(win, baseAttributes, configObj) {
/** @const @private {!Window} */
this.win_ = win;

/** @const @private {!Object<string, string>} */
/** @const @private {!JsonObject<string, string>} */
this.baseAttributes_ = baseAttributes;

/** @const @private {!JSONType} */
Expand Down Expand Up @@ -83,14 +84,15 @@ export class AnchorAdStrategy {

placeStickyAd_() {
const viewportWidth = viewportForDoc(this.win_.document).getWidth();
const attributes = Object.assign({}, this.baseAttributes_, {
'width': String(viewportWidth),
'height': '100',
});
const attributes = /** @type {!JsonObject} */ (
Object.assign(dict(), this.baseAttributes_, dict({
'width': String(viewportWidth),
'height': '100',
})));
const ampAd = createElementWithAttributes(
this.win_.document, 'amp-ad', attributes);
const stickyAd = createElementWithAttributes(
this.win_.document, 'amp-sticky-ad', {'layout': 'nodisplay'});
this.win_.document, 'amp-sticky-ad', dict({'layout': 'nodisplay'}));
stickyAd.appendChild(ampAd);
this.win_.document.body.insertBefore(
stickyAd, this.win_.document.body.firstChild);
Expand Down
15 changes: 8 additions & 7 deletions extensions/amp-auto-ads/0.1/attributes.js
Expand Up @@ -15,6 +15,7 @@
*/

import {user} from '../../../src/log';
import {dict} from '../../../src/utils/object';
import {startsWith} from '../../../src/string';
import {isArray, isObject} from '../../../src/types';

Expand All @@ -29,26 +30,26 @@ const NON_DATA_ATTRIBUTE_WHITELIST = {
};

/**
* @param {!Object} configObj
* @return {!Object<string, string>}
* @param {!JsonObject} configObj
* @return {!JsonObject<string, string>}
*/
export function getAttributesFromConfigObj(configObj) {
if (!configObj['attributes']) {
return {};
return dict();
}
if (!isObject(configObj['attributes']) || isArray(configObj['attributes'])) {
Copy link
Contributor

@wassgha wassgha Jun 22, 2017

Choose a reason for hiding this comment

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

Is this still enough to qualify for JsonObject? Also, might need to change the warning text.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since this is a not, it is actually sound. Unfortunately it is essentially impossible to detect the correct type at runtime. We do have cases that definitely get it now.
My changes for now, try to limit themselves to cases that the type system can detect.

user().warn(TAG, 'attributes property not an object');
return {};
return dict();
}
return parseAttributes(configObj['attributes']);
}

/**
* @param {!Object} attributeObject
* @return {!Object<string, string>}
* @param {!JsonObject} attributeObject
* @return {!JsonObject<string, string>}
*/
function parseAttributes(attributeObject) {
const attributes = {};
const attributes = dict();
for (const key in attributeObject) {
if (!NON_DATA_ATTRIBUTE_WHITELIST[key] && !startsWith(key, 'data-')) {
user().warn(TAG, 'Attribute not whitlisted: ' + key);
Expand Down
15 changes: 8 additions & 7 deletions extensions/amp-auto-ads/0.1/placement.js
Expand Up @@ -15,6 +15,7 @@
*/

import {dev, user} from '../../../src/log';
import {dict} from '../../../src/utils/object';
import {getAttributesFromConfigObj} from './attributes';
import {resourcesForDoc} from '../../../src/services';
import {
Expand Down Expand Up @@ -88,7 +89,7 @@ export class Placement {
* @param {!Element} anchorElement
* @param {!Position} position
* @param {!function(!Element, !Element)} injector
* @param {!Object<string, string>} attributes
* @param {!JsonObject<string, string>} attributes
* @param {!../../../src/layout-rect.LayoutMarginsChangeDef=} opt_margins
*/
constructor(win, resources, anchorElement, position, injector, attributes,
Expand All @@ -108,7 +109,7 @@ export class Placement {
/** @const @private {!function(!Element, !Element)} */
this.injector_ = injector;

/** @const @private {!Object<string, string>} */
/** @const @private {!JsonObject<string, string>} */
this.attributes_ = attributes;

/**
Expand Down Expand Up @@ -164,7 +165,7 @@ export class Placement {
}

/**
* @param {!Object<string, string>} baseAttributes Any attributes to add to
* @param {!JsonObject<string, string>} baseAttributes Any attributes to add to
* injected <amp-ad>. Specific attributes will override defaults, but be
* overridden by placement specific attributes defined in the
* configuration.
Expand Down Expand Up @@ -194,16 +195,16 @@ export class Placement {
}

/**
* @param {!Object<string, string>} baseAttributes
* @param {!JsonObject<string, string>} baseAttributes
* @return {!Element}
* @private
*/
createAdElement_(baseAttributes) {
const attributes = Object.assign({
const attributes = /** @type {!JsonObject} */ (Object.assign(dict({
'layout': 'fixed-height',
'height': '0',
'class': 'i-amphtml-layout-awaiting-size',
}, baseAttributes, this.attributes_);
}), baseAttributes, this.attributes_));
return createElementWithAttributes(
this.win_.document, 'amp-ad', attributes);
}
Expand Down Expand Up @@ -231,7 +232,7 @@ export function getPlacementsFromConfigObj(win, configObj) {
* Validates that the placementObj represents a valid placement and if so
* constructs and returns an instance of the Placement class for it.
* @param {!Window} win
* @param {!Object} placementObj
* @param {!JsonObject} placementObj
* @param {!Array<!Placement>} placements
*/
function getPlacementsFromObject(win, placementObj, placements) {
Expand Down
2 changes: 1 addition & 1 deletion extensions/amp-call-tracking/0.1/amp-call-tracking.js
Expand Up @@ -32,7 +32,7 @@ let cachedResponsePromises_ = {};
* Fetches vendor response.
* @param {!Window} win
* @param {!string} url
* @return {!Promise<Object>}
* @return {!Promise<JsonObject>}
*/
function fetch_(win, url) {
if (!(url in cachedResponsePromises_)) {
Expand Down
9 changes: 5 additions & 4 deletions src/analytics.js
Expand Up @@ -22,6 +22,7 @@ import {createElementWithAttributes} from './dom';
import {getAmpdoc} from './service';
import {extensionsFor} from './services';
import {dev} from './log';
import {dict} from './utils/object';


/**
Expand Down Expand Up @@ -79,15 +80,15 @@ export function insertAnalyticsElement(
const doc = /** @type {!Document} */ (parentElement.ownerDocument);
const analyticsElem = createElementWithAttributes(
doc,
'amp-analytics', {
'amp-analytics', dict({
'sandbox': 'true',
'trigger': 'immediate',
});
}));
const scriptElem = createElementWithAttributes(
doc,
'script', {
'script', dict({
'type': 'application/json',
});
}));
scriptElem.textContent = JSON.stringify(config);
analyticsElem.appendChild(scriptElem);
analyticsElem.CONFIG = config;
Expand Down
2 changes: 1 addition & 1 deletion src/base-element.js
Expand Up @@ -932,7 +932,7 @@ export class BaseElement {
* @note Boolean attributes have a value of `true` and `false` when
* present and missing, respectively.
* @param {
* !Object<string, (null|boolean|string|number|Array|Object)>
* !JsonObject<string, (null|boolean|string|number|Array|Object)>
* } unusedMutations
*/
mutatedAttributesCallback(unusedMutations) {
Expand Down