Webviews: Two-way messaging #46
Conversation
| @@ -1,19 +1,29 @@ | |||
| <!DOCTYPE html> | |||
There was a problem hiding this comment.
I wouldn't give this page too much scrutiny, as it's going away imminently. I just updated for completeness sake.
| @@ -24,7 +24,7 @@ export class AuthorizeCommand { | |||
| if (!result || result.title === l10n.t('No')) { | |||
| return Promise.resolve(false); | |||
| } else { | |||
| await commands.executeCommand('sfdx.force.auth.web.login'); | |||
| await commands.executeCommand('sfdx.org.login.web'); | |||
There was a problem hiding this comment.
This is actually broken in the current code in main. 😔 Apparently the command names moved on and we didn't know it.
src/webviews.ts
Outdated
| delete data.callbackId; | ||
| callback = (responseData?: object) => { | ||
| const fullResponseMessage = { | ||
| callbackId: returnedCallbackId, |
|
Approach looks good to me! |
|
Got good coverage on the webview helper class I pulled out ( Still need to look at how I can test the JS side of the messaging, but that's for another day. |
src/webviews/processor.ts
Outdated
| (messageHandler) => data.type === messageHandler.type | ||
| ); | ||
| if (responsiveHandlers.length > 0) { | ||
| const handler = responsiveHandlers[0]; |
There was a problem hiding this comment.
nit: Refactor into a new file(processor.ts) hides the assertion that why index 0 handler is a valid handler. Since the validation of handler map takes place at all the caller(showInstructionWebview) maybe onWebviewReceivedMessage should be passed a single message handler instead of an array there?
There was a problem hiding this comment.
That's a good catch! In my next PR I'm actually moving the handler validation into the processor as well, for the reason you stated and the reason that it's really more congruent there (another aspect you pointed out).
While I like having the array just passed in here for its own processing, I do agree that the selection of just the first item feels a little oblique. I could either add a comment here, or what I'm leaning toward is just switching to messageHandlers.find() instead, which only returns the first item found (which is always going to be 0 or 1 items), and probably reads better for this use case. Let me know what you think.
khawkins
force-pushed
the
two_way_messaging
branch
from
0f61031
to
5028e0b
Compare
In order to support the logic of the forthcoming landing page selection and LWC QA generation, we needed a two-way channel between the webviews and their backing TypeScript controller pages. This does that. I took out the previous one-way button event messaging, and replaced it with a more general two-way protocol.
This is feature complete, but needs tests. But I wanted to get the concept in front of you all for any feedback you wish to offer.