-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
[adr] BiDi low-level definitions are generated from a shared spec model without orchestration #17701
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
Open
titusfortner
wants to merge
5
commits into
SeleniumHQ:trunk
Choose a base branch
from
titusfortner:adr-bidi-cddl
base: trunk
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+101
β0
Open
[adr] BiDi low-level definitions are generated from a shared spec model without orchestration #17701
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
6d22642
[docs] add BiDi generated protocol layer design decision record
titusfortner 4b3f61d
[docs] number BiDi ADR record by PR and link discussion
titusfortner a49e5ab
[docs] set ADR title and discussion link to PR 17701
titusfortner 67b42c9
[docs] retitle ADR: definitions generated without orchestration
titusfortner 6ce1f6e
[docs] clarify generated modules may call single commands; orchestratβ¦
titusfortner File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,101 @@ | ||
| # 17701. BiDi's low-level definitions are generated from a shared spec model without orchestration | ||
|
|
||
| - Status: Proposed | ||
| - Discussion: https://github.com/SeleniumHQ/selenium/pull/17701 | ||
|
|
||
| ## Context | ||
|
|
||
| The WebDriver BiDi specification is defined in CDDL (Concise Data Definition Language). | ||
| A binding's BiDi support spans four layers, from the wire up: | ||
|
|
||
| - **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 | ||
| (including the id types β a subscription, an intercept β and the commands that produce and consume | ||
| them), exposed by generated modules that can execute a single command. | ||
| - **Orchestration** β the stateful coordination across those commands and events: storing a | ||
| subscription to unsubscribe later, mapping an intercept id to a handler, matching an event to its | ||
| registered callback, and wrapping events into the objects a handler receives. | ||
| - **High-level API** β the protocol-neutral, idiomatic capabilities users program against. | ||
|
|
||
| Bindings differ today in how they produce the definitions and how cleanly they separate them from | ||
| the orchestration above: | ||
|
|
||
| | Binding | Current behavior | | ||
| |------------|------------------| | ||
| | Java | Hand-written (~143 module classes); definitions and orchestration on the same class; separate protocol-neutral high-level (`RemoteNetwork`). | | ||
| | Python | Generated from CDDL; orchestration injected into the generated classes via an enhancements manifest. | | ||
| | Ruby | Hand-written low-level; orchestration fused with the high-level API. | | ||
| | .NET | Hand-written module classes. | | ||
| | JavaScript | Generated from CDDL; orchestration injected into the generated classes via an enhancements manifest. | | ||
|
|
||
| They also differ on source of truth β the CDDL spec or the existing implementation β and on whether | ||
| they share one model or each interpret CDDL independently. | ||
|
|
||
| This record decides the low-level definitions layer β what it is, where it comes from, and how the | ||
| layers around it relate to it. This ADR assumes #17670 is accepted and that BiDi is an internal | ||
| implementation. | ||
|
|
||
| ## Decision | ||
|
|
||
| **1. The spec is the oracle, through one shared model.** The definitions are generated from a | ||
| single shared, binding-neutral projection of the spec β not reconstructed from the existing | ||
| implementation, and not parsed independently per binding. Because the spec, not the existing code, | ||
| defines the shape, the generated definitions need not match the existing implementation β in API | ||
| shape or byte-for-byte. The shared model is the one place the spec is interpreted and normalized, so | ||
| bindings stay consistent with each other; each still emits its own language-idiomatic code from it. | ||
|
|
||
| **2. Generated data objects are immutable; generated modules may call single commands.** The | ||
| generated protocol data objects β command parameters and results, event payloads β are immutable | ||
| and information-only. Generated domain modules may expose directly callable single-command methods | ||
| (e.g. a generated `BrowsingContext.navigate(...)` that executes one command) as internal, unsupported | ||
| implementation APIs β not the supported, user-facing surface, which is the high-level API. | ||
|
|
||
| **3. Stateful orchestration stays out of the generated layer.** Generated modules execute individual | ||
| commands, but own no stateful coordination: subscriptions, event dispatch, handler and intercept-id | ||
| mappings, body collectors, and lifecycle management live in the orchestration layer, which imports | ||
| the generated definitions rather than being spliced into them (e.g. through an enhancements manifest). | ||
| A thin convenience over a single command may be generated; what must not land here is coordination. | ||
| The generated definitions stay a projection of the spec, so regenerating them never disturbs the | ||
| orchestration that imports them, and they depend only on the transport's send-and-deliver interface. | ||
|
|
||
| ## Considered options | ||
|
|
||
| - **Keep hand-maintaining the definitions** β each binding writes and updates the protocol types, | ||
| commands, and events by hand, with no generation. | ||
| - The same protocol is hand-maintained separately in every binding, so they drift apart β the | ||
| inconsistency this record exists to prevent. | ||
| - Every spec change is a manual edit repeated in each binding, with no shared source of truth. | ||
|
|
||
| - **Put the orchestration in the generated low-level class (e.g. via an enhancements manifest)** β | ||
| splice the coordination β subscription lifecycle, dispatch, handler wrapping β into the generated | ||
| classes alongside the spec types, as Python and JavaScript do today. | ||
| - Couples two layers that change for different reasons: a spec update and a coordination change | ||
| touch the same artifact, and regenerating risks the coordination. | ||
| - Coordination is harder to find, review, and type-check when it lives inside generated output. | ||
| - Thin conveniences are not the concern β the objection is coordination in the low-level layer. | ||
|
|
||
| - **Derive the definitions from the existing implementation** β generate to | ||
| reproduce the current shape. | ||
| - Treats the existing implementation as the source of truth instead of the spec, carrying its | ||
| inconsistencies forward. | ||
| - Nothing supported depends on the generated definitions, so they need not match it. | ||
|
|
||
| - **Generate each binding independently from CDDL** β every binding walks the CDDL/AST and builds | ||
| its own model with its own logic, rather than consuming one shared model. | ||
| - The grouping of modules, commands, and events follows the spec, but the normalization, naming, | ||
| and gap handling are re-derived per binding, so they drift apart over time. | ||
| - A shared, binding-neutral model makes those decisions once; each binding still emits | ||
| language-idiomatic code from it. | ||
|
|
||
| ## Consequences | ||
|
|
||
| - What users program against is the high-level API, not the generated definitions, so regenerating | ||
| from a changed spec does not change it. | ||
| - The generated definitions can live in their own namespace that the higher layers migrate onto, | ||
| and the existing implementation is retired. | ||
| - Orchestration and the high-level API are checked-in source, navigable and reviewable, and | ||
| regenerating the definitions never touches them. | ||
| - A binding that today combines the layers in one class (with injected orchestration and | ||
| enhancements) splits them: the generated definitions move to their own namespace, which the | ||
| orchestration and high-level API import. | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Where is this "shared model"? What form does it have? Who owns 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.
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