-
Notifications
You must be signed in to change notification settings - Fork 4
Make using renderers and react-console work; support multiple RunmeConsoles #28
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
base: main
Are you sure you want to change the base?
Conversation
…tedError: Failed to execute 'define' on 'CustomElementRegistry': the name close-cell-button has already been used with this registry
|
@sourishkrout I'm slightly confused by how rendering works; if I'm understanding codex; in the current version
Is that right?
|
|
@sourishkrout Any thoughts on how we could make it easy to build a testApp that allows us to use mocks/fakes for the actual Runner? I started building out a small testApp the goal being to have 3 different consoles; 2 Runme and 1 ConsoleView each talking to different backends. Do you think there's a way to move all the streams logic into a separate class with a simple API for pushing/recieving messages from the console that would allow us to easily fake it out? |
|
@sourishkrout Here's more info on what I'm ultimately trying to do. I want the webapp to be configured with multiple runners; e.g. one running locally and another running in a K8s cluster. The AppConsole comes in as a way of configuring the runners. Rather than continuing to build out the settings page I want to add an app console where one can simple run commands e.g. |
|
@sourishkrout so I initially hit the issues with ConsoleView which I believe were the result of renderers not being declared a peer dependency resulting in multiple instances of the renderers package being present leading to the error due to custom elements being redefined. While fixing that I started noticing the use of the shared context bridge and started wondering if that would cause problems in the event we want to have RunmeConsole's talking to different runners. I've been trying to tease this apart. Here's what I understand from ChatGPT and looking at the code. In VSCode extensions
Now it seems like RunmeConsole doesn't actually rely on the RenderContext for communicating with the kernel. However; in installCtxBridge RunmeConsole is calling setContext to set the shared RenderContext. I think each RunmeConsole would override whatever was already set; so we'd have a last one wins situation. The RenderContext looks like it is only used to send ExecuteRequests containing the window size. I think there is a bug here in the case where each RunnerConsole could be talking to a different runner because the request would get sent to whatever #streams were being used as a result of the last RunnerConsole to call SetContext. However, given this is only used to set window size I'm not sure how much this matters. |
|
This fixes the isolated issue where |
That's what streams.ts does. |
I think the solution is to namespace the RenderContext where non-Runme Runner backed have their own, individually or shared, RenderContext. Looking into this right now. |
|
@sourishkrout Is #29 ; using safeCustomElement the right solution? codex originally came up with that same solution (e5a5bd5) and I ultimately rejected it. Won't multiple imports of the module lead to ill defined behavior and subtle bugs? My simple understanding is that a custom-element can only be registered once. So presumably, if we have two different instances of the module renderersA and renderersB; the custom element gets bound to the implementation / JS code in one or the other. This will lead to problems e.g. if the versions aren't the same. Or in our case since we were relying on a module level variable (context: RenderContext) I think we wind up with problems because some code is using renderserA and other code is using renderersB and its not being consistent. |
|
@sourishkrout I think this will work and here's how it compares to #29 and #30
|
No. For the purposes of registering custom elements inside the DOM and avoiding collisions, it is a suitable solution. The barrel export/import from a consumer's convenience point of view is well worth preserving which has the side-effect of running the registration logic on every import. Since the DOM is a singleton we just lean on it and very little to no cost. Even multiple instance of the same code (idempotent) is not a problem. The |
I'm fine using the "Streams interface" if that's more convenient. What matters most to me is that both Runme web and the extension continue to work which both are relying parties. |
|
|
||
| // Optional Streams factory for testing/custom backends | ||
| @property({ attribute: false }) | ||
| StreamCreator?: (props: StreamsProps) => StreamsLike |
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.
why upper-case/PascalCase?
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.
That's what codex did.
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 should it be?
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.
Camel-case like the others: streamCreator...
|
Sorry for the slow reply.
Don't we risk version skew if we have more than one version? I think node_modules would end up looking like this.
Is there a better pattern here to make it more testable? Do we want to allow injecting Runme client fakes? What's a good way to make sure both runmeweb and vscode continue to work? Can you remind me why we have 2 webcomponents (console-view and runme-console)? It looks like RunmeConsole is binding the console-view to a particular implementation of the logic. Is that just a convenience? Is it necessary for vscode? I think an alternative would be you just have console-view and you bind it to different handlers; such as a Runme handler. (I think this is where the Streams idea is headed). |
|
I pushed a dev version from this branch and started using it; seems to be working. I find myself needing to implement a line-editor to have the terminal behave as expected (e.g. arrow keys navigate the line; backspace edit the values). Is that expected. codex is telling me that's not provided by xterm.js; xterm.js just provides raw stdin. How does runme-console handle this when you start a shell such as bash? Or maybe it doesn't need to because the actual terminal is handling that for you? |
I believe you're right that is indeed a possibility. However, it's best-practice to use a single version across all packages/modules in a mono-repo of this size. Inevitably, the problem remains the same, that you can only register a single component under the same name. To avoid, that problem, one could let the consumer of the either To avoid the scenario above, both transitive deps should be bumped to
Possibly, more on that in response to your question below.
Yes, necessary on one hand because VS Code's architecture is set but it's also flexible enough to run inside alternative client architectures, such as "the web app" in this repo.
The PR I drafted essentially uses |
Makes sense.
xterm.js and its more concrete derivative If I understand your goal correctly, I'd recommend using the same "frontend" (RunmeConsole + Streams + ExecuteRequest) but create an alternative ExecuteRequest implementation (possibly via a separate gRPC service using the same/wrapped types) and then hook it up to an "terminal app" built using https://github.com/charmbracelet/bubbletea. It should be straightforward to have the service stream an "atomic" config update (e.g. |
|
@sourishkrout Thank you. I've fallen down a rabbit hole. The TL;DR is
Here's what happened
|
|
I've stripped this PR down to the bare minimum needed to make my app work. There's really only 2 changes in this PR
Of these I think only the first really matters to me. I've changed my app (at least for now) to only use ConsoleView and only import renderers. I guess we'll find out whether this makes sense. I'm not blocked so we can take our time / wait till after the holiday Make MessageBridge a per context attribute alternative to #30What's the thinking in #30 of using namespaces and a module level map to keep track of the bridge? As opposed to making them per module? I'm slightly confused about what purpose the RendererContext servers in the web-app case. Why do we need the MessageBridge vs just reading from the terminal directly? IUC messaging is a bit asysmetric
Even with my changes I found the behavior to be somewhat brittle
The idea of a namespaced RenderContext as in #29 is confusing to me if we are relying on the RenderContext to wire up stdin. I would have thought each console would need their own to have their own stdin? Peer DependencyBased on my grasp of the various issues; I think marking renderers a peer dependency of the others so that the app has a single instance is the correct way to solve the issue of the custom element being rendered multiple times. That said if you prefer to go with #29 I can just revert this. Since I'm not depending on react-console any more its not a huge concern for me. Code Reuse In WebAppsI'm not sure whether dropping down to console-view is the right thing to do in a webapp but here's some of why I did it. I wanted more control over the stream creation and I wanted that to be independent of the visual/react components. For web app could we
is react-console intended for webapps or vscode primarily? |
| external: [ | ||
| '@buf/googleapis_googleapis.bufbuild_es', | ||
| '@runmedev/renderers', | ||
| '@runmedev/react-console', |
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.
Don't think it'll work to externalize these. The webapp just imports @runmedev/react-components and you shouldn't have to provide these separately. Is this strictly necessary?
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.
You're probably right. I can try reverting this.
Since I'm just importing react-console we can probably revert all the peer dependency changes.
I'm okay with the peer dependency inside the workspace. However, I don't think we should externalize unless there's a very good reason for it.
A variation of I'm not quite sure why you'd have to go down all the way down to
Again, I think a more flexible I'm okay with fine with merging an optional per instance context bridge injection. I'm just not sure if it's really needed. |
|
I think I can just revert the peer dependency changes.
Specifically if you look at my screenshot; my UI consists of the "input panels" (cell contents, runner, language) and the "output panels" (CellConsole). The way the input panels communicate with the output (CellConsole) is via the proto. I found this architecture to be at odds with the way Console & RunmeConsole is architected. IUC; in Console most of the properties needed for execution (cellID, runID, language) are passed as arguments to the Console component.
I had a hard time wrapping my mind around this and decided to rearchitect it.
I'm generally trying to decouble all the business logic from REACT/rendering concerns. My hypothesis is that this will lead to a more testable design e.g.
e.g. to the earlier point
It seems strange to me that the instantiation of the runner clients is managed by the REACT/UI component. Now that users are selecting runners by name e.g "local" which is mapped to some endpoint e.g. wss://localhost:9988/ws. You need to do late binding; i.e. wait until the cell is executed to map local to its endpoint. At that point you can just as easily construct the streams and wire it up the observable streams to the terminal. Context-Bridge Injection
I'm still very confused by context bridge. It looks like that's how the terminal sends messages and in particular stdin to the app. Without it I couldn't capture stdin and pass them along to runme. Canonical example was being to start a shell e.g. by executing "bash" and then get an interactive shell in the cellconsole. My bridge ends up looking like. |
I think I'm tracking. With the introduction of the output registry, I refactored the terminal console's rendering to be proto-reactive. E.g. based on the presence of
I'm a bit confused. You might be conflating Web and React Components "interchangeable" which they are not. The composition hierarchy is below and makes sense because it allows HTML-native components and React/Vue/Svelte derivatives to share core functionality. flowchart TD
A["<b>console-view</b><br/><i>Web Component</i><br/>━━━━━━━━━━━━━━<br/>xterm.js with styling,<br/>addons, and<br/>x-sandbox messaging"]
B["<b>runme-console</b><br/><i>Web Component</i><br/>━━━━━━━━━━━━━━<br/>Websockets messaging<br/>with Golang-backed Runner"]
C["<b>Console</b><br/><i>React Component</i><br/>━━━━━━━━━━━━━━<br/>Thin wrapper for<br/>WC -> React"]
D["<b>CellConsole</b><br/><i>React Component</i><br/>━━━━━━━━━━━━━━<br/>Console <> Cell Proto<br/>Coupling"]
A --> B
B --> C
C --> D
classDef webc fill:#e1f5ff,stroke:#01579b,stroke-width:2px
classDef reactc fill:#f3e5f5,stroke:#4a148c,stroke-width:2px
class A,B webc
class C,D reactc
Again, in my mind (ignoring ContextBridge) there's no need to touch anything below the React-fold because it's not beholden to React's reactive rendering loop. The APIs for those components aren't set in stone and for breaking changes, it's easy enough to write one's own.
Using vanilla DOM events here is just a way to make keyboard shortcuts work across the whole app and avoid prop-drilling. Ideally running a cell would be part of
It's not. I agree that it would be "wrong" to that. That's why
As said above, this is entirely a concern of Bottom-line: Some minor refactoring to the context bridge notwithstanding (prototype holdover), above's architecture cleanly separates concerns for both testability and working across VS Code, this webapp, and doc sites integrations etc. These layers of indirection might seem unintuitive in a pure React application. However, outside of React ecosystem lock-in it would add costly rendering loop reconciliation cost that only slows the lower non-app layers down. |
Correct. The API is broader than what is being used. The fact that it was/is set as "singleton" is a holdover from the prototype days where getting something working was paramount. The webcomponents are reusing VS Code's Renderer Messaging API for two reasons: a) it's stable API to x-sandbox (Javascript runtime) communicate without rolling our own, and b) it allows bringing existing renders, e.g. Jupyter's Renderers (plots, tables, etc) to brought into the webapp. The API itself is documented here: https://code.visualstudio.com/api/extension-guides/notebook#notebook-renderer. |


Motivation
AppConsole
We'd like to be able to use ConsoleView to create an AppConsole. The AppConsole will be a terminal that will allow interacting with the app itself; e.g. managing configuration.
The problem is if try to import @runmedev/renderers we end up with an error
So we need to allow embedding the Runme console alongside other UI without CustomElement collisions or renderer context crashes
RunmeConsole's with different backends
We'd like to have RunmeConsole's talking to different backends. Currently that doesn't work because there is a single RenderContext attached to the underlying ConsoleView.
Bug:
The webcomponent console-view was being registered twice when both @runmedev/react-console and @runmedev/renderers were imported, leading to custom element name clashes; the console also crashed with “Renderer context not defined” when bundled copies of renderers diverged.
I think there are two issues
runmedev/renderers and runmedev/console both were causing the side effect of registering the custom components
the console-view uses a module level render context;
Fix:
Make custom element registration idempotent, externalize/dedupe @runmedev/renderers/@runmedev/react-console as peers to share a single instance, add a safe fallback for missing renderer context
ConsoleView per instance RenderContext
Allow ConsoleView to have per instance RenderContext rather than always relying on the module level RenderContext.
For backwards compatibility continue to fall back to calling getContext if the per instance context isn't set.