Refactor observables in viewer-impl into a map object by muxin · Pull Request #7004 · ampproject/amphtml

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

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

src/service/viewer-impl.js

    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

  /** @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

  /** @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

  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

      return Promise.resolve();
    }
    if (eventType == 'broadcast') {
-      this.broadcastObservable_.fire(


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

src/service/viewer-impl.js

-   * @param {function(!JSONType)} handler
-   * @return {!UnlistenDef}
-   */
-  onBroadcast(handler) {


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

test/functional/test-runtime.js

      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

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


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

test/functional/test-viewer.js

    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

    }
+    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


    /** @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

Conversation

muxin commented Jan 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

muxin commented Jan 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

muxin commented Jan 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants