proposal: add entrypoint-based custom export api - #114
Conversation
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is a clean, well-documented API proposal, and the entrypoint-based approach is the right shape: it keeps the export handler off the gadget's public RPC surface (only the workshop backend can call it) and reuses the existing named-WorkerEntrypoint mechanism the hooks path already uses (getGadgetHookEntrypoint → stub.getEntrypoint(name) in overseer.ts). The getExportFormats(gadget) / export(gadget, id) signatures taking a Fetcher<Gadget> match the facet-passing pattern that exportPdf/connectToGadget already use. Since this is explicitly for discussion, here's design-level feedback rather than a merge review.
The naming collision with "output formats" is the biggest concern
The kernel already has a first-class concept called output format (BlueprintOutput, OutputFormatOffer, listOutputFormats(), the AdminFormat/Formats admin panel, packages/workshop-frontend/src/components/format/). That's "what a blueprint produces" (Document / Slides / Spreadsheet). This PR introduces GadgetExportFormat / GadgetClientExportFormat / getExportFormats() meaning something entirely different (file export targets: HTML/PDF/CSV/...).
Two things called "format" that are unrelated will be a persistent source of confusion for reviewers and future readers of the kernel. Worth considering GadgetExportTarget / ExportOption / getExportOptions(), or otherwise disambiguating in the type names — not just the doc comments.
How does GadgetClientExportApi attach to the RPC surface?
GadgetClient (api.ts:2724) already owns exportPdf(chatId?), and the "use" role doc (api.ts:2824) specifically enumerates getUiBundle(), connectToGadget(), and exportPdf() as the use-role surface. It's unclear how the new GadgetClientExportApi relates:
- Are
getExportFormats/exportintended to be added toGadgetClient(replacing/supersedingexportPdf)? If so, the proposal should say so, and address the migration ofexportPdf(deprecate? keep as sugar forexport("pdf")?). - Or is
GadgetClientExportApia separate capability returned by some accessor? Then, per the RPC conventions here, it shouldextend RpcTarget(every other client-facing RPC interface in this file does —WorkpieceClient,PublicApi,Overseer, etc.), and the proposal should show where it's minted.
As written it's a bare interface with no RpcTarget and no attachment point, so a reader can't tell how the client actually reaches it. Given exportPdf already lives on GadgetClient with the same chatId? parameter, folding these two methods into GadgetClient and retiring exportPdf seems like the most natural fit and avoids a parallel capability.
globalThis.gadgetExportFormatId deserves scrutiny
Browser-mode variants signal the selected id via globalThis.gadgetExportFormatId in the remotely-rendered UI. A few questions for the design:
- The current renderer (
browser-export.ts) serves a locked-down CSP document and injects the gadget'sjsCode. Where/when isgadgetExportFormatIdset relative to the gadget's own bootstrap, and is it guaranteed to be readable before the gadget renders? A global that races the app's first paint is fragile. globalThison an untyped magic name is awkward to consume from a typed gadget. Consider whether this belongs in the same injected surface the gadget already gets, rather than a free-floating global — and if it stays, ship an ambient declaration so gadget authors get types.- Naming: everything else here is "export format"; the global is
gadgetExportFormatId. Keep them consistent with whatever the type rename lands on.
Smaller points
getExportFormatsuniqueness across modes is a contract the backend must enforce. The doc says ids "must be unique across both modes" and that a gadget "should return" the default browser formats unless opting out. Since a gadget authors this, the backend needs to (a) reject/de-dupe colliding ids and (b) decide what happens when the entrypoint is present but returns[]or omits the method — worth stating the backend's validation and fallback rules in the proposal, since that's kernel behavior reviewers will care about.- Default-format opt-out semantics. "return these ... unless it specifically does not want to support browser-mode HTML or PDF" — so the defaults are injected only when
ExportHandler/getExportFormatsis absent, and a present handler is fully authoritative (can drop PDF)? Please make that explicit; it's the crux of the "disable/reimplement defaults" feature bullet. - HTML browser-mode renderer doesn't exist yet.
browser-export.tsonly implementsrenderGadgetPdf;contentType: "text/html"implies a new capture path. Fine for a proposal, but flag it as required follow-up work (and note the CSP/settle guarantees must hold for HTML capture too). export()returnsReadableStream<Uint8Array>but carries no filename.GadgetClientExportFormathaslabel+fileExtension; the frontend'smakeExportFilename(gadgetTitle, ext)presumably builds the name. Confirm the custom-mode handler never needs to influence the filename beyond the extension (e.g. a report named by date).- Stream limits / abuse. Custom-mode
export()returns a gadget-authored stream. The PDF path enforcesMAX_EXPORT_DURATION_MS/MAX_EXPORT_BYTES. The same bounding must wrap custom streams (gadget code is untrusted); worth noting in the proposal. - Doc-comment consistency. Per the repo's kernel bar, every exported member of the shared public API needs a doc comment.
GADGET_EXPORT_ENTRYPOINThas one; good. The two block comments inside theGadgetExportFormatunion use//inline comments rather than/** */on the members — minor, but the reviewers here read every line of shared API changes. export.tsimportsDurableObjectonly for theGadgetdefault type param. Fine, just confirming it's intentional thatGadgetExportEntrypoint<Gadget extends DurableObject = DurableObject>defaults to the baseDurableObject(the gadget's real DO class isn't known in shared code).
Overall: the capability model (backend-only caller, entrypoint not on the public RPC interface, defaults-when-absent) is sound and matches how hooks already work. The main things to resolve before this becomes an implementation PR are the "format" naming collision with the existing output-format concept and how GadgetClientExportApi attaches to the client RPC surface vs. the existing exportPdf.
|
Posted a design review on PR #114. Summary of what I flagged: Positives
Main design concerns
Smaller points: backend must enforce id-uniqueness/validation and define the defaults opt-out semantics; the HTML browser-mode renderer doesn't exist yet ( No code changes were made — this PR is explicitly a proposal for discussion, so I reviewed it as a design. |
maxwellpeterson
changed the title
There was a problem hiding this comment.
Follow-up on the current revision. Since the last review the doc comments tightened up several of the earlier concerns — getExportFormats is now stated as mandatory (Workshop rejects a missing method rather than applying defaults), GadgetClientExportMethods explicitly folds into GadgetClient and supersedes exportPdf, and the 30s / 100MB bounds are now called out for custom streams. Good. A few things remain worth resolving before this becomes an implementation PR.
The "format" naming collision is still the biggest issue
The kernel already owns a first-class output format concept — BlueprintOutput / OutputFormatOffer / Overseer.listOutputFormats() (api.ts:420, api.ts:1063, api.ts:1076), the AdminFormat/Formats admin panel, and workshop-frontend/src/components/format/. That's "what a blueprint produces" (Document / Slides / Spreadsheet). This PR reuses the same word for "file export targets" (HTML/PDF/CSV). getExportFormats() on the client will sit a few hundred lines from listOutputFormats() on the same Overseer/client surface, and both return ...Format[]. That's a real hazard for the kernel reviewers who read every line here.
Recommend disambiguating in the type names, not just prose: GadgetExportTarget / GadgetClientExportTarget / getExportTargets() (or ExportOption / getExportOptions()), and rename the magic global to match whatever lands.
GadgetClientExportMethods folds into GadgetClient — verify the two consumers of exportPdf
The doc now commits to replacing GadgetClient.exportPdf() (api.ts:2745). When this is implemented, two spots must move together:
- The
"use"role doc at api.ts:2824-2825 enumeratesexportPdf()as part of the use-role surface — it needs to namegetExportFormats()/export()instead. overseer.tshas twoexportPdfimplementations (overseer.ts:9054 and overseer.ts:9301). Both are the backend caller thatrenderGadgetPdfbacks.
Since export.ts is a standalone module, it can't structurally enforce that GadgetClient actually adopts GadgetClientExportMethods. Consider having GadgetClient extends GadgetClientExportMethods directly (in api.ts) so the intent is checked by the compiler rather than living only in a comment in a sibling file.
globalThis.gadgetExportFormatId vs. the actual render path
Grounding this against browser-export.ts: renderGadgetPdf responds to the navigation request with makeExportHtml(clientCode) under a locked-down CSP (browser-export.ts:293-300), opens the RPC session, then waitForDomSettled (browser-export.ts:308-312). For the global to be reliable it has to be set before the gadget's bundle executes — i.e. injected into makeExportHtml ahead of clientCode, not set post-load — otherwise it races first paint. Please state that in the design, and ship an ambient declare global so gadget authors reading it get a type instead of consuming an untyped magic name.
Browser-mode is PDF-only today
Confirming the follow-up scope: browser-export.ts only implements renderGadgetPdf (PDF via page.createPDFStream, browser-export.ts:318). contentType: "text/html" needs a new capture path that reuses the same deadline / MAX_EXPORT_BYTES / releaseWhenSettled machinery (browser-export.ts:249, 324) and the same CSP/settle guarantees. Worth flagging as required work in the proposal.
Smaller points
- Custom-mode stream bounding is documented but must be wired. The 30s/100MB text on
export()matchesMAX_EXPORT_DURATION_MS/MAX_EXPORT_BYTES, but those constants are currently private tobrowser-export.ts(browser-export.ts:14,16). Since a gadget-authored custom stream is untrusted, the samelimitStream+ deadline wrapping must apply on the custom path — probably by lifting those limits/helpers to a shared spot. - Doc-comment style on the union members. The three-way union in
GadgetExportFormatstill has leading//section comments ("Gadget-facing export API:", "Export API exposed to the Workshop UI:"). The exported members are documented with/** */, which meets the bar; just confirming the inline//dividers are intentional stylistic separators and not meant to document exported members. getExportFormats()returning[]or an all-custom list. The doc covers "method absent → defaults" and "method present → authoritative (may drop PDF)". Please also pin down: (a) is an empty array legal (gadget exposes no export options, UI hides the button)? and (b) backend id de-dup/validation of the returned list, since a gadget authors it.Fetcher<Gadget extends DurableObject = DurableObject>default to the baseDurableObjectis fine for shared code (the real DO class isn't known here) — matches thegetGadgetHookEntrypointpattern in overseer.ts. No change needed, just confirming.
Net: the capability shape (backend-only caller, entrypoint off the public RPC surface, defaults-when-absent) is right and the recent doc tightening resolved most of the earlier ambiguity. The naming collision and the mechanical exportPdf → export/getExportFormats migration (both overseer.ts sites + the "use" role doc) are the two things I'd want nailed down before implementation.
|
Review posted to PR #114. Since this is an explicitly design-only proposal (150 additions, purely additive/unwired), I reviewed it as a design rather than making code changes. My follow-up focused on what the current revision (SHA What's now resolved in the current revision (so I didn't re-raise): What I flagged as still open, grounded in the current code:
|
|
UnknownError: ProviderInitError |
|
@maxwellpeterson Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
|
Follow-up on the current revision (SHA A few things remain open before this becomes an implementation PR. One stale name left in proseThe rename is complete in the type/method signatures, but the
(export.ts:89-90) — should read
|
|
Review posted to PR #114. Since this is an explicitly design-only proposal (purely additive/unwired, 2 files), I reviewed the current revision (SHA What's now resolved (so I didn't re-raise):
What I flagged as still open:
|
|
Follow-up on the current revision (SHA Re-grounding the rest against the current code, nothing else changed, so the standing items are unchanged: Still open (all fine for a proposal; flagging as the implementation-PR checklist)
Confirmed / no change needed
Net: the two biggest items from prior rounds (the "format" naming collision and the stale prose) are both closed. What remains is the implementation-PR checklist above — the |
|
Review posted to PR #114. Since the last review, one new commit ( Newly resolved (verified):
Confirmed still resolved: the "format" naming collision (types are Still open (implementation-PR checklist, fine for a proposal):
The two biggest items across all rounds (naming collision + stale prose) are now closed. |
1 participant
This PR proposes a new file export API for Gadgets. Currently, Gadgets can only be exported to PDF by rendering their UI in a remote browser and printing the page:
We want Gadgets to be able to register their own custom export formats. For example, a document editor Gadget could support Markdown, HTML, and PDF exports, and a data analysis Gadget could support CSV and XLSX exports:
The API interface described in this PR allows Gadgets to customize their own export behavior, and has several desirable attributes:
This PR exists to illustrate the proposed API interface. It's meant for discussion, and not intended to be merged as-is.