-
Notifications
You must be signed in to change notification settings - Fork 215
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.
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
feat(checkout): CHECKOUT-7538 Introduce extension messenger and command handler #2050
Conversation
658bc5e
to
efe284a
Compare
efe284a
to
593abbd
Compare
9630507
to
ebe0b53
Compare
* @param extensionId - The ID of an extension sending the commands. | ||
* @param handlers - A set of handlers for the extension commands. | ||
*/ | ||
addExtensionCommandHandlers(extensionId: string, handlers: ExtensionCommandHandlers): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to return a function that could unsubscribe the handler, similar to the existing subscribe
method. e.g.:
listenExtensionCommand(extensionId: string, commandType: ExtensionCommand, commandHandler, ExtensionCommandHandler): () => void
This makes it easier to unsubscribe a handler, as calling the return value can specifically remove such handler. Otherwise, clients need to hold a reference to the handler and therefore can't use inline anonymous function as the handler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated it as:
listenExtensionCommand(
extensionId: string,
command: ExtensionCommand,
handler: ExtensionCommandHandler,
): () => void
To entirely remove the command listening, I also added a method in the checkoutService
to stop listening for an extension.
stopListenExtensionCommand(extensionId: string): void
private _posters: { [extensionId: string]: IframeEventPoster<HostOriginEvent> } = {}; | ||
|
||
listen( | ||
extensions: Extension[], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking you can just inject ReadableCheckoutStore
as a constructor parameter of ExtensionMessenger
. That way you don't need to pass extensions
. You can just call the following to get the available extensions.
const { data: { getExtensions } } = this.store.getState();
const extensions = getExtensions();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Now the constructor is:
constructor(
private _store: CheckoutStore,
private _listeners: { [extensionId: string]: IframeEventListener<ExtensionOriginEventMap> },
private _posters: { [extensionId: string]: IframeEventPoster<HostOriginEvent> },
) {}
e0aac1f
to
24beff1
Compare
24beff1
to
e8ae208
Compare
* @alpha | ||
* @param extensionId - The ID of the extension that originally sent the commands. | ||
*/ | ||
stopListenExtensionCommand(extensionId: string): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could skip this method for now unless we really need it. IframeEventListener#stopListen
doesn't actually remove the references to the handlers previously registered. So we need to do a bit of work to actually do what we want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed. We can circle back to it once we have the use case.
private _extensions: Extension[] | undefined; | ||
|
||
constructor( | ||
private _store: CheckoutStore, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preferably use the read-only version, i.e.: ReadableCheckoutStore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, ReadableCheckoutStore
is used in ways I was not aware of before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic work @bc-peng, LGTM! 👏
What?
Why?
checkout-js
will be able to carry out the corresponding actions when receving a command from an extension.Testing / Proof