Refactor observables in viewer-impl into a map object #7004

muxin · 2017-01-12T00:34:48Z

jridgewell · 2017-01-12T00:39:33Z

src/service/viewer-impl.js

@@ -115,21 +115,15 @@ export class Viewer {
    /** @private {number} */
    this.paddingTop_ = 0;

+    /** @private {!Object<string, !Observable<!JSONType>>} */
+    this.messageObservables_ = [];


Is this supposed to be an object, or an array?

Array it is~ Will fix this.

Actually I want it to be a map of observables, the index is string. Which type should I annotate here?

this.messageObservables_ = {}; ?

Use a prototype-less object:

import {map} from 'src/utils/object.js'; /** @private {!Object<string, !Observable<!JSONType>>} */ this.messageObservables_ = map();

Thanks for the suggestion. Fixed.

dvoytenko · 2017-01-12T18:41:05Z

extensions/amp-access/0.1/amp-access.js

@@ -339,7 +339,7 @@ export class AccessService {

  /** @private */
  listenToBroadcasts_() {
-    this.viewer_.onBroadcast(message => {
+    this.viewer_.onMessage('broadcast', message => {


Broadcast shouldn't change - it should stay as it was onBroadcast. Broadcast is a special type of messaging, thus we want a special API for it.

Probably be the easiest to just revert this file and related other.

dvoytenko · 2017-01-12T18:42:46Z

src/service/storage-impl.js

@@ -136,7 +136,7 @@ export class Storage {

  /** @private */
  listenToBroadcasts_() {
-    this.viewer_.onBroadcast(message => {
+    this.viewer_.onMessage('broadcast', message => {


Ditto: revert

dvoytenko · 2017-01-12T18:42:53Z

extensions/amp-access/0.1/test/test-amp-access.js

@@ -740,7 +740,7 @@ describe('AccessService authorization', () => {

  it('should run authorization for broadcast events on same origin', () => {
    let broadcastHandler;
-    sandbox.stub(service.viewer_, 'onBroadcast', handler => {
+    sandbox.stub(service.viewer_, 'onMessage', (eventType, handler) => {


Ditto: revert

dvoytenko · 2017-01-12T18:43:20Z

src/service/viewer-impl.js

-    this.historyPoppedObservable_ = new Observable();
-
-    /** @private {!Observable<!JSONType>} */
-    this.broadcastObservable_ = new Observable();


dvoytenko · 2017-01-12T18:43:47Z

src/service/viewer-impl.js

-   * @param {function(ViewerHistoryPoppedEventDef)} handler
+   * Adds a eventType listener for viewer events.
+   * @param {string} eventType
+   * @param {function(T)} handler


This will always be the ViewerMessage. No need for a template.

dvoytenko · 2017-01-12T18:54:48Z

src/service/viewer-impl.js

@@ -787,7 +779,7 @@ export class Viewer {
      return Promise.resolve();
    }
    if (eventType == 'broadcast') {
-      this.broadcastObservable_.fire(


dvoytenko · 2017-01-12T18:54:53Z

src/service/viewer-impl.js

-   * @return {!UnlistenDef}
-   */
-  onBroadcast(handler) {
-    return this.broadcastObservable_.add(handler);


dvoytenko · 2017-01-12T18:55:20Z

test/functional/test-runtime.js

@@ -909,7 +909,7 @@ describes.realWin('runtime multidoc', {
      const amp = win.AMP.attachShadowDoc(hostElement, importDoc, docUrl);
      const viewer = getServiceForDoc(ampdoc, 'viewer');
      const broadcastReceived = sandbox.spy();
-      viewer.onBroadcast(broadcastReceived);
+      viewer.onMessage('broadcast', broadcastReceived);


dvoytenko · 2017-01-12T18:55:30Z

test/functional/test-storage.js

@@ -41,7 +41,7 @@ describe('Storage', () => {

    viewerBroadcastHandler = undefined;
    viewer = {
-      onBroadcast: handler => {
+      onMessage: (eventType, handler) => {


dvoytenko · 2017-01-12T18:55:44Z

test/functional/test-viewer.js

@@ -500,7 +500,7 @@ describe('Viewer', () => {

    it('should receive broadcast event', () => {
      let broadcastMessage = null;
-      viewer.onBroadcast(message => {
+      viewer.onMessage('broadcast', message => {


muxin · 2017-01-13T00:44:34Z

@jridgewell @dvoytenko PTAL

dvoytenko · 2017-01-13T01:01:04Z

src/service/viewer-impl.js

-   * @param {function(ViewerHistoryPoppedEventDef)} handler
+   * Adds a eventType listener for viewer events.
+   * @param {string} eventType
+   * @param {!function(!JSONType)} handler


functions are already ! by default. remove.

dvoytenko · 2017-01-13T01:02:48Z

src/service/viewer-impl.js

@@ -791,6 +770,11 @@ export class Viewer {
          /** @type {!JSONType|undefined} */ (data));
      return Promise.resolve();
    }
+    const observable = this.messageObservables_[eventType];
+    if (observable) {
+      observable.fire(/** @type {!JSONType} */ (data));


It's already !JSONType. Why cast it again?

dvoytenko · 2017-01-13T01:03:34Z

src/service/viewport-impl.js

@@ -108,7 +108,7 @@ export class Viewport {
    this./*OK*/scrollLeft_ = null;

    /** @private {number} */
-    this.paddingTop_ = viewer.getPaddingTop();
+    this.paddingTop_ = viewer.getInitPaddingTop();


Here: this.paddingTop_ = Number(viewer.getParam('paddingTop') || 0). Should work, right?

Yeah, thanks for the simpler solution

dvoytenko · 2017-01-13T01:04:30Z

src/service/viewer-impl.js

-      this.historyPoppedObservable_.fire({
-        newStackIndex: data['newStackIndex'],
-      });
+      this.messageObservables_[eventType].fire(


Actually, thinking more about it. We can definitely easily add return type to our observable handler so this is already future-proof. So, agree with you, let's skip ViewerMessage.

dvoytenko · 2017-01-13T01:07:15Z

src/service/viewport-impl.js

-    const duration = event['duration'] || 0;
-    const curve = event['curve'];
+  updateOnViewportEvent_(data) {
+    const paddingTop = data['paddingTop'];


According to old code, it's possible that paddingTop is undefined. I.e. see this old code snippet:

if (eventType == 'viewport') { if (data['paddingTop'] !== undefined) { this.paddingTop_ = data['paddingTop']; this.viewportObservable_.fire( /** @type {!JSONType} */ (data));

I don't know if it's a real situation here. If it is, we may want to add the following in this method:

const paddingTop = data['paddingTop']; if (paddingTop == undefined) { return; }

Please check.

yeah, fixed

muxin · 2017-01-13T20:58:19Z

@jridgewell PTAL

* master: (310 commits) Update csa.md to remove non-required parameters (ampproject#6902) Add notes about requesting ads ATF and link to demo (ampproject#7037) Remove whitelist for lightbox scrollable validator (ampproject#7034) Delegate submit events until amp-form is loaded (ampproject#6929) Moves closure sha384 into a new extension amp-crypto-polyfill for lazy load (ampproject#7006) Refactor observables in viewer-impl into a map object (ampproject#7004) resizing of margins (ampproject#6824) Use URL replacer from embed for pixel (ampproject#7029) adds support for Gemius analytics (ampproject#6558) Avoid duplicating server-layout (ampproject#7021) Laterpay validator config (ampproject#6974) Validator rollup (ampproject#7023) skeleton for amp-tabs (ampproject#7003) Upgrade post-css and related packages to latest (ampproject#7020) handle unload (ampproject#7001) viewer-integr.js -> amp-viewer-integration (ampproject#6989) dev().info()->dev().fine() (ampproject#7017) Turned on experiment flag (ampproject#6774) Unlaunch ios-embed-wrapper for iOS8 to avoid scroll freezing issues (ampproject#7018) Add some A4A ad request parameters (ampproject#6643) ...

muxin assigned dvoytenko Jan 12, 2017

muxin requested a review from dvoytenko January 12, 2017 00:34

jridgewell requested changes Jan 12, 2017

View reviewed changes

dvoytenko suggested changes Jan 12, 2017

View reviewed changes

muxin added 3 commits January 12, 2017 12:56

Refactor observables in viewer-impl into a map object

ec0a4b1

Fix check-types

c43eb90

Keep broadcastObservable and rename paddingTop_ in viewer

e5191c9

muxin force-pushed the refactor-observables branch from 6b668a3 to e5191c9 Compare January 13, 2017 00:01

dvoytenko reviewed Jan 13, 2017

View reviewed changes

dvoytenko suggested changes Jan 13, 2017

View reviewed changes

No need to store paddingTop value in viewer-impl

14d3f39

dvoytenko approved these changes Jan 13, 2017

View reviewed changes

jridgewell approved these changes Jan 13, 2017

View reviewed changes

muxin merged commit c882ba1 into ampproject:master Jan 13, 2017

muxin deleted the refactor-observables branch January 13, 2017 21:21

jridgewell pushed a commit to jridgewell/amphtml that referenced this pull request Jan 31, 2017

Refactor observables in viewer-impl into a map object (ampproject#7004)

91f9c85

mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017

Refactor observables in viewer-impl into a map object (ampproject#7004)

4f5dd5c

This was referenced Sep 16, 2020

🚨 Error: Animation.animate is not a function #30253

Closed

🚨 Error: Animation is not defined #30335

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor observables in viewer-impl into a map object #7004

Refactor observables in viewer-impl into a map object #7004

muxin commented Jan 12, 2017

jridgewell Jan 12, 2017

muxin Jan 12, 2017

muxin Jan 12, 2017

muxin Jan 12, 2017

jridgewell Jan 12, 2017

muxin Jan 13, 2017

dvoytenko Jan 12, 2017

muxin Jan 13, 2017

dvoytenko Jan 12, 2017

dvoytenko Jan 12, 2017

dvoytenko Jan 12, 2017

dvoytenko Jan 12, 2017

dvoytenko Jan 12, 2017

dvoytenko Jan 12, 2017

dvoytenko Jan 12, 2017

dvoytenko Jan 12, 2017

dvoytenko Jan 12, 2017

muxin commented Jan 13, 2017

dvoytenko Jan 13, 2017

muxin Jan 13, 2017

dvoytenko Jan 13, 2017

muxin Jan 13, 2017

dvoytenko Jan 13, 2017

muxin Jan 13, 2017

dvoytenko Jan 13, 2017

dvoytenko Jan 13, 2017

muxin Jan 13, 2017

muxin commented Jan 13, 2017

Refactor observables in viewer-impl into a map object #7004

Refactor observables in viewer-impl into a map object #7004

Conversation

muxin commented Jan 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muxin commented Jan 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muxin commented Jan 13, 2017