[adr] BiDi low-level definitions are generated from a shared spec model without orchestration#17701
[adr] BiDi low-level definitions are generated from a shared spec model without orchestration#17701titusfortner wants to merge 5 commits into
Conversation
PR Summary by QodoAdd ADR for generating BiDi low-level protocol definitions from shared spec model Description
Diagram
High-Level Assessment
Files changed (1)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
11 rules 1. No binding-status section
|
|
Code review by qodo was updated up to the latest commit 4b3f61d |
|
Code review by qodo was updated up to the latest commit a49e5ab |
titusfortner
changed the title
|
Code review by qodo was updated up to the latest commit 67b42c9 |
diemol
left a comment
diemol
left a comment
There was a problem hiding this comment.
I was not aware of the consequences of each binding doing their own thing. I believe It makes sense to unify this.
|
|
||
| ## Decision | ||
|
|
||
| **1. The spec is the oracle, through one shared model.** The definitions are generated from a |
There was a problem hiding this comment.
Where is this "shared model"? What form does it have? Who owns it?
There was a problem hiding this comment.
Do we need to define it for the ADR or leave it as an implementation detail?
What I meant is what we're doing in #17700
It's taking the cddl npm to generate an AST, walks it to generate a bidi-model, takes the model and the ast and generates a schema that can be directly used by the bindings without additional walking/parsing.
This is currently done with bazel targets in javascript/selenium-webdriver that are marked visible to other bindings
|
I think to understand this better, I might beed an example of what the proposed shared spec model looks like. |
|
|
||
| - **Transport / session substrate** — the connection, sending each command and correlating its | ||
| response by the envelope id, and delivering inbound event frames upward. Domain-blind. | ||
| - **Low-level definitions** — the types, command shapes, and event shapes the spec defines, |
There was a problem hiding this comment.
Just for clarification, this means if we add a new CDDL for a different part of the web platform that we can't offer anything callable until we have the orchestration/high level agreed. The current python code at least makes the low level stuff callable and gives us an opportunity for later high level/orchestration
There was a problem hiding this comment.
if we can have a meaningful way to make things callable then we can close #17709 as I think we can assume the layer 1 is a given in that ADR, Layer 2 is the orchestration part, Layer 3 is callable, it doesn't need to in those modules but it shouldn't be overly hard to use. E.g. we have permissions CDDL in the tree, we're working on the ADR for high level/orchestration but the primitives are already being built.
The callable primitives gives us an edge over competing tools since we're allowing people to keep up with the web platform as it is being written.
There was a problem hiding this comment.
Ok, yes, I wrote this in a confusing way. It should say that it generates immutable data objects for working with protocol responses, not that the generated code shouldn't generate callable command methods with parameters.
The boundary this ADR is proposing is that generated modules may execute individual commands, but they do not own stateful coordination such as subscriptions, event dispatch, handler mappings, intercept IDs, collectors, or lifecycle management.
I’ll update the decision to say:
Generated protocol data objects are immutable. Generated domain modules may expose directly callable single-command methods as unsupported implementation APIs. Stateful orchestration and lifecycle management remain in separate handwritten code.
So permissions primitives would be callable as soon as the CDDL is generated; a supported permissions API can be designed later. Does that address the callability concern behind #17709?
For instance, the Ruby implementation I'm working on supports this in our Bridge:
def get(url)
BiDi::Protocol::BrowsingContext.new(self).navigate(context: window_handle, url: url, wait: readiness)
endWhere BrowsingContext class is auto-generated from cddl schema and looks something like:
def navigate(context:, url:, wait: UNSET)
@transport.execute(cmd: 'browsingContext.navigate', params: NavigateParameters.new(context: context, url: url, wait: wait), result: BrowsingContext::NavigateResult)
end
produces: |
|
Code review by qodo was updated up to the latest commit 6ce1f6e |
|
After the update you made, I now understand what this ADR is trying to address. I think it makes sense. For most part of JS CDDL generation , it follows this ADR. The manifest only has some convenience methods that hand-written code created, mainly to maintain backward compatibility (which we can remove to align with this ADR once the TLC agrees on it). |
|
I only wonder where the access to unsubscribing to an event should live. |
|
The subscribe/unsubscribe commands are in the spec, so they're generated like any other command and directly callable on the generated module. What lives outside the generated code is the management: holding the subscription id, mapping it to a handler, and the cancellation behavior we want. The shape of that orchestration is decided elsewhere. |
|
As far as the JS implementation, the schema from #17700 would be a replacement for cddl2ts and require less follow-up work. Haven't looked into it in much detail, but that's the idea is that it is better shaped for what we need. |
4 participants
📄 The decision, its rationale, considered options, and consequences are in the record file
this PR adds; read it there. The sections below are proposal notes and review logistics.
🔗 Related
📝 Proposal notes
🗣 TLC discussion
Pending — not yet discussed.
📌 Tracking
Tracking issue: (linked on acceptance)