Export imperative API from carousel#30046
Conversation
samouri
left a comment
There was a problem hiding this comment.
I'm new to useImperativeHandle, and this seems like a clean pattern for exposing APIs from a component! One request: can you add small description for this PR?
Left some non-blocking comments. Only major item is the .only Caroline found.
| * @param {../action-constants.ActionTrust} minTrust | ||
| * @protected | ||
| */ | ||
| registerApiAction(alias, handler, minTrust) { |
There was a problem hiding this comment.
would we make this a no-op in the bento mode envelope?
There was a problem hiding this comment.
I don't think so. We still need to allow action execution. E.g. a caller should be able to call carousel.goToSlide(0) no matter what mode we are in.
There was a problem hiding this comment.
Does this mean we would want a Bento version of amp-state? I don't recall having discussions about this
There was a problem hiding this comment.
No need for Bento amp-state. But the on-actions are not part of amp-state - that's just normal imperative API for an element. Just like video.pause() - there has to be a way to pause a video programmatically. Otherwise the usability of an element is significantly reduced. This is also usually not a big deal to support - it'd most often be a simple passthrough element.goToSlide -> api(component).goToSlide.
There was a problem hiding this comment.
To clarify, amp-state might use element on-actions, but these actions exist independent of amp-state.
There was a problem hiding this comment.
And one more clarification. I also don't expect Bento mode to use on="tap:carousel.goToSlide" type of thing. But somehow the element needs to export an API to make it possible still or the element's API surface will be incomplete. Currently it's possible with element.enqueAction('goToSlide') - a really complicated way to do it, but sufficient for testing. We will be looking into how we can make these API calls more ergonomic in Bento world.
There was a problem hiding this comment.
API calls more ergonomic in Bento world
This PR sets the stage for it! One potential option would be to forward all keys within the .getApi() object to the element, so the caller could write element.goToSlide
There was a problem hiding this comment.
Thanks for clarifying, this all makes sense and I think the imported independently was groggy Monday brain. Feel free to disregard.
There was a problem hiding this comment.
I think forwarding is exactly what we should do. But Not yet 100% certain it'd be that simple. Another option is to simply export api property on the element. E.g. users would call element.api.goToSlide(). That has two benefits:
- We could give
.apimore clear async semantics. It's not available until the element is upgraded. - We don't ever have to worry about method collision with any DOM APIs - it's basically functions as a namespace.
But it has one big downside: per Preact semantics the imperative handle can easily change, which is not something a Bento user would always expect. element.api@time1 !== element.api@time2.
Thus, I think, some sort of proxying is a more likely solution for us. This is something to fully design and TODO, but not subject for this PR specifically since it's only taking care of the element->preact bridge.
There was a problem hiding this comment.
I created ampproject/wg-bento#36 to track the element API question.
* Export imperative API from carousel * review fixes
This pull request uses Preact's
useImperativeHandleto export the component's imperative API to the AMP layer. The most common use for this is support of AMP actions. This pull request implementsnext,prev, andgoToSlideactions. The imperative API itself is declared and described in thetype.jsfile.Partial for: #28284
Related to: ampproject/wg-bento#36