From 08c3f49ea5fa0e033d3b16e275679054e17a5e8d Mon Sep 17 00:00:00 2001
From: Ali Ghassemi <aghassemi@google.com>
Date: Fri, 16 Sep 2016 13:54:30 -0700
Subject: [PATCH] Type-check for <amp-viz-vega> (#5008)

---
 build-system/amp.extern.js                  | 12 ++++
 extensions/amp-viz-vega/0.1/amp-viz-vega.js | 68 +++++++++++++--------
 gulpfile.js                                 |  2 +-
 3 files changed, 55 insertions(+), 27 deletions(-)

diff --git a/build-system/amp.extern.js b/build-system/amp.extern.js
index 0986e43a3137..f584f3ec12c3 100644
--- a/build-system/amp.extern.js
+++ b/build-system/amp.extern.js
@@ -42,6 +42,18 @@ window.AMP_CONFIG.thirdPartyFrameRegex;
 window.AMP_CONFIG.cdnUrl;
 window.AMP_CONFIG.errorReportingUrl;
 
+// amp-viz-vega related externs.
+/**
+ * @typedef {{spec: function(!JSONType, function())}}
+ */
+let VegaParser;
+/**
+ * @typedef {{parse: VegaParser}}
+ */
+let VegaObject;
+/* @type {VegaObject} */
+window.vg;
+
 // Should have been defined in the closure compiler's extern file for
 // IntersectionObserverEntry, but appears to have been omitted.
 IntersectionObserverEntry.prototype.rootBounds;
diff --git a/extensions/amp-viz-vega/0.1/amp-viz-vega.js b/extensions/amp-viz-vega/0.1/amp-viz-vega.js
index 89d40ba9bf1a..1de38381f4e4 100644
--- a/extensions/amp-viz-vega/0.1/amp-viz-vega.js
+++ b/extensions/amp-viz-vega/0.1/amp-viz-vega.js
@@ -30,30 +30,24 @@ const EXPERIMENT = 'amp-viz-vega';
 
 export class AmpVizVega extends AMP.BaseElement {
 
-  /** @override */
-  isLayoutSupported(layout) {
-    return isLayoutSizeDefined(layout);
-  }
-
-  /** @override */
-  buildCallback() {
-    user().assert(isExperimentOn(this.win, EXPERIMENT),
-        `Experiment ${EXPERIMENT} disabled`);
+/** @param {!AmpElement} element */
+  constructor(element) {
+    super(element);
 
     /** @private {?JSONType} */
     this.data_ = null;
 
-    /** @const @private {?string} */
-    this.inlineData_ = this.getInlineData_();
+    /** @private {?string} */
+    this.inlineData_ = null;
 
-    /** @const @private {?string} */
-    this.src_ = this.element.getAttribute('src');
+    /** @private {?string} */
+    this.src_ = null;
 
-    /** @const @private {boolean} */
-    this.useDataWidth_ = this.element.hasAttribute('use-data-width');
+    /** @private {boolean} */
+    this.useDataWidth_ = false;
 
-    /** @const @private {boolean} */
-    this.useDataHeight_ = this.element.hasAttribute('use-data-height');
+    /** @private {boolean} */
+    this.useDataHeight_ = false;
 
     /** @private {number} */
     this.measuredWidth_ = 0;
@@ -61,18 +55,38 @@ export class AmpVizVega extends AMP.BaseElement {
     /** @private {number} */
     this.measuredHeight_ = 0;
 
-    /**
-     * @const @private {!Object}
-     * Global vg (and implicitly d3) are required and they are created by
-     * appending vega and d3 minified files during the build process.
-     */
-    this.vega_ = this.win.vg;
+    /** @private {?VegaObject} */
+    this.vega_ = null;
+
+    /** @private {?Element} */
+    this.container_ = null;
 
     /**
      * @private {Object}
      * Instance of Vega chart object. https://goo.gl/laszHL
      */
     this.chart_ = null;
+  }
+
+  /** @override */
+  isLayoutSupported(layout) {
+    return isLayoutSizeDefined(layout);
+  }
+
+  /** @override */
+  buildCallback() {
+    user().assert(isExperimentOn(this.win, EXPERIMENT),
+        `Experiment ${EXPERIMENT} disabled`);
+
+    /**
+     * Global vg (and implicitly d3) are required and they are created by
+     * appending vega and d3 minified files during the build process.
+     */
+    this.vega_ = this.win.vg;
+    this.inlineData_ = this.getInlineData_();
+    this.src_ = this.element.getAttribute('src');
+    this.useDataWidth_ = this.element.hasAttribute('use-data-width');
+    this.useDataHeight_ = this.element.hasAttribute('use-data-height');
 
     user().assert(this.inlineData_ || this.src_,
         '%s: neither `src` attribute nor a ' +
@@ -144,7 +158,9 @@ export class AmpVizVega extends AMP.BaseElement {
       // point to other Vega specs) an they don't include credentials on those
       // calls. We may want to intercept all "urls" in spec and do the loading
       // and parsing ourselves.
-      return xhrFor(this.win).fetchJson(this.src_).then(data => {
+
+      return xhrFor(this.win).fetchJson(dev().assertString(this.src_))
+      .then(data => {
         this.data_ = data;
       });
     }
@@ -186,7 +202,7 @@ export class AmpVizVega extends AMP.BaseElement {
 
     return parsePromise.then(chartFactory => {
       return vsyncFor(this.win).mutatePromise(() => {
-        dom.removeChildren(this.container_);
+        dom.removeChildren(dev().assertElement(this.container_));
         this.chart_ = chartFactory({el: this.container_});
         if (!this.useDataWidth_) {
           const w = this.measuredWidth_ - this.getDataPadding_('width');
@@ -206,7 +222,7 @@ export class AmpVizVega extends AMP.BaseElement {
   /**
    * Gets the padding defined in the Vega data for either width or height.
    * @param {!string} widthOrHeight One of 'width' or 'height' string values.
-   * @return {!Number}
+   * @return {!number}
    * @private
    */
   getDataPadding_(widthOrHeight) {
diff --git a/gulpfile.js b/gulpfile.js
index 8621676517ea..b2384320bcde 100644
--- a/gulpfile.js
+++ b/gulpfile.js
@@ -98,7 +98,7 @@ declareExtension('amp-twitter', '0.1', false);
 declareExtension('amp-user-notification', '0.1', true, 'NO_TYPE_CHECK');
 declareExtension('amp-vimeo', '0.1', false, 'NO_TYPE_CHECK');
 declareExtension('amp-vine', '0.1', false, 'NO_TYPE_CHECK');
-declareExtension('amp-viz-vega', '0.1', true, 'NO_TYPE_CHECK');
+declareExtension('amp-viz-vega', '0.1', true);
 declareExtension('amp-google-vrview-image', '0.1', false, 'NO_TYPE_CHECK');
 declareExtension('amp-youtube', '0.1', false);