-
Notifications
You must be signed in to change notification settings - Fork 12
feat: first communication between extension and webview #11
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
Conversation
Signed-off-by: Philippe Martin <[email protected]>
71ac88f to
26319f2
Compare
Signed-off-by: Philippe Martin <[email protected]>
| } | ||
|
|
||
| // Broadcast events (sent by extension and received by the webview) | ||
| export const ACTIVE_RESOURCES_COUNT = createRpcChannel<undefined>('ActiveResourcesCount'); |
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.
AFAIK it should not be <undefined>
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.
What do you think? Should it be some 'empty' value (void?), or do you think it is expecting some real data? (I would prefer to not pass data from here, but let the frontend query the data when it wants, with some throttling/debouncing)
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.
RPC channel is used by the webview to call methods on the extension so I would not expect an undefined type at this level. It's providing at the end a Proxy.
I expect here to have an API (an interface) and then this interface can have Promise methods.
webview then say: Give me a proxy to counters API
and then I can ask: startCounting(): Promise<void> (it's a dummy example)
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.
In the lightspeed extension (https://github.com/redhat-developer/podman-desktop-redhat-lightspeed-ext/blob/main/packages/common/src/channels.ts), I understand that the pattern you are describing is used for the RPC channels (first section), but not for the Broadcast events (second section), where the type is only the data the extension wants to send to the webview along with the event. Am I wrong?
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.
for broadcast sections, then it's broadcasting the value to be displayed
so for the case of counts for example it should be ActiveResourcesCountInfo interface with for example count: number field
the idea is that the frontend is never asking directly a data
frontend ask for a data, backend broadcast the value of the data, frontend receive the value and can display it.
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.
My argument for doing the other way (webview asking for the data), is that the data provided by the extension will be massive (for some events, it will be the complete description of all the pods, for example). We will need to introduce some debouncing/throttling to limit how frequently we really want to transfer this data. Do you think that this should be done in the extension side instead of the webview?
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.
my idea is that we don't broadcast every 5s for example (or on every change) or we don't need to display the value
but the logic is: frontend ask when it's interesting, then extension/backend will send the data. Debouncing/throttling being done on the extension side, webview only taking care of displaying values.
webview side:
I want to monitor cluster 2 for "pod resource"
and then it receives value for "pods"
you change the page, then it's sending "I'm no longer interested" and the data are no longer sent.
overall idea is:
only quick calls from frontend to backend (we're almost never waiting for a result)
and sending data is only done asynchronously from backend to frontend.
logic = backend, display = frontend
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.
or in short: we ask for a value but we don't get it directly, it's received through another channel.
it avoids stores /states that follow the logic: if not intialized, asked for the data.
initEvents(): {
if (!events) {
events = await window.grabLatestEvents();
}
onEvents(events: Events) {
events = events)
}
in fact, we don't have anymore grabLatestEvents methods at all that return the value
values are always received through the events
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.
ok, I'll move all the logic to the extension. Thanks
| * SPDX-License-Identifier: Apache-2.0 | ||
| ***********************************************************************/ | ||
|
|
||
| import type { ContextsStatesDispatcher } from '../manager/contexts-states-dispatcher'; |
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.
/@ imports
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 changed to use /@ everywhere. But I'm nit sure what is the guideline: should it be used everywhere, or do we want to keep import './...' when importing files in the current directory (or subdirectories)?
Signed-off-by: Philippe Martin <[email protected]>
|
replaced by #17 |
listenActiveResourcesCountfrom Podman Desktop and adapts it to use RPCExtensionActiveResourceCountto demo the previous function(I'm working on the next PR which should replace the original (from PD)
listenActiveResourcesCountand all stores withStates (as in RedHat Lightspeed ext)