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.

Sign up for GitHub

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

Jump to bottom

Implement support for the amp-bind premutate message #12906

Merged
merged 7 commits into from Jan 24, 2018
Merged

Implement support for the amp-bind premutate message #12906

merged 7 commits into from Jan 24, 2018

Conversation

josh313
Copy link
Contributor

@josh313 josh313 commented Jan 19, 2018

Closes #12811

@choumx

@dreamofabear dreamofabear self-requested a review January 19, 2018 01:04
dreamofabear
dreamofabear reviewed Jan 23, 2018
View reviewed changes
src/service/viewer-impl.js Outdated
this.messageObservables_[eventType] = observable;
}
return observable.add(handler);
this.messageHandlers_[eventType] = handler;
Copy link

Choose a reason for hiding this comment

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

Does this mean that a given eventType can only have one handler? Not sure that's desirable or safe -- let's retain the current behavior if possible.

How about adding an optional parameter to Observable#fire such that it returns Promise.all(<invoked_handlers>) if provided?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that would work because then the promise would return an array, and that's not what we want to send in the response. I updated this code now to keep the onMessage method as is, and add a separate onMessageRespond method that makes the provided function the sole handler that responds to the message. So any given message type can have one responder, but multiple other observers.

extensions/amp-bind/0.1/bind-impl.js Outdated
/**
* @param {string} eventType
* @param {*} data
* @private
Copy link

Choose a reason for hiding this comment

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

@return {!Promise}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

extensions/amp-bind/0.1/bind-impl.js Outdated
premutate_(eventType, data) {
const ignoredKeys = [];
return this.initializePromise_
.then(() => {
Copy link

Choose a reason for hiding this comment

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

Nit: This can fit on the previous line.

extensions/amp-bind/0.1/bind-impl.js
@@ -333,6 +337,39 @@ export class Bind {
return this.history_;
}

/**
Copy link

Choose a reason for hiding this comment

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

"Calls setState(s), where s is data.state with non-overridable keys removed."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@dreamofabear
Copy link

/cc @honeybadgerdontcare Validator change FYI.

honeybadgerdontcare
honeybadgerdontcare approved these changes Jan 24, 2018
View reviewed changes
Copy link
Contributor

@honeybadgerdontcare honeybadgerdontcare left a comment

Choose a reason for hiding this comment

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

validation changes look good

dreamofabear
dreamofabear approved these changes Jan 24, 2018
View reviewed changes
Copy link

@dreamofabear dreamofabear left a comment

Choose a reason for hiding this comment

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

Oh, are you planning to return error info from premutate_? I assume this will be in the next PR?

extensions/amp-bind/0.1/bind-impl.js Outdated
* @return {!Promise}
* @private
*/
premutate_(eventType, data) {
Copy link

Choose a reason for hiding this comment

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

Are you planning to use awaitResponse?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I didn't end up using it. I removed eventType and awaitResponse from the function signature for the handlers now.

@josh313
Copy link
Contributor Author

josh313 commented Jan 24, 2018

@choumx I'm not planning on returning error info (I assume you mean non-overridable keys?) from premutate_. I thought we decided in the design review not to do that, and to just log them here instead. If we decide to do it, I can do it in a follow up PR.

@dreamofabear
Copy link

Just asking because I wondered why Promise.all(observableHandlers) didn't suffice. It implies that you need the return value of one of the handlers instead of just waiting for the promise. Or am I missing something?

@josh313
Copy link
Contributor Author

josh313 commented Jan 24, 2018

I guess you're right that would have possibly worked in this case, although I think it probably would have returned an an array with just one undefined item in it, which isn't good. And this solution seems cleaner since there is a clear way to define what does go in the response and which handlers do the responding.

@josh313
Copy link
Contributor Author

josh313 commented Jan 24, 2018

Obviously, feel free to merge if you think it's ready. I'm not able to.

@dreamofabear dreamofabear merged commit aa74b48 into ampproject:master Jan 24, 2018
@dreamofabear
Copy link

Thanks for contributing this!

@honeybadgerdontcare honeybadgerdontcare mentioned this pull request Jan 26, 2018
Merged
honeybadgerdontcare added a commit that referenced this pull request Jan 26, 2018
@honeybadgerdontcare
Validator Rollup (#13045)
7147e7e 
* Revision bump for #12862

* Revision bump for #13001

* Revision bump for #12906
gzgogo pushed a commit to gzgogo/amphtml that referenced this pull request Jan 26, 2018
@josh313 @gzgogo
Implement support for the amp-bind premutate message (ampproject#12906)
3f24f71 
* Implement support for the amp-bind premutate message

* fix unit tests

* Fix lint

* Revert changes to onMessage and instead add a new onMessageRespond method

* remove eventType and awaitResponse from RequestResponder parameters
gzgogo pushed a commit to gzgogo/amphtml that referenced this pull request Jan 26, 2018
@honeybadgerdontcare @gzgogo
Validator Rollup (ampproject#13045)
a112207 
* Revision bump for ampproject#12862

* Revision bump for ampproject#13001

* Revision bump for ampproject#12906
RanAbram pushed a commit to RanAbram/amphtml that referenced this pull request Mar 12, 2018
@josh313 @RanAbram
Implement support for the amp-bind premutate message (ampproject#12906)
c9a66ff 
* Implement support for the amp-bind premutate message

* fix unit tests

* Fix lint

* Revert changes to onMessage and instead add a new onMessageRespond method

* remove eventType and awaitResponse from RequestResponder parameters
RanAbram pushed a commit to RanAbram/amphtml that referenced this pull request Mar 12, 2018
@honeybadgerdontcare @RanAbram
Validator Rollup (ampproject#13045)
e01cd27 
* Revision bump for ampproject#12862

* Revision bump for ampproject#13001

* Revision bump for ampproject#12906
protonate pushed a commit to protonate/amphtml that referenced this pull request Mar 15, 2018
@honeybadgerdontcare
Validator Rollup (ampproject#13045)
c47e924 
* Revision bump for ampproject#12862

* Revision bump for ampproject#13001

* Revision bump for ampproject#12906
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@honeybadgerdontcare honeybadgerdontcare honeybadgerdontcare approved these changes

@dreamofabear dreamofabear dreamofabear approved these changes

Assignees
No one assigned
Labels
cla: yes PR use: In Stable
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@josh313 @dreamofabear @honeybadgerdontcare @googlebot @ampprojectbot