RFC-0006: Notebook Typescript API#1334
Conversation
b67e3e7 to
91c94ba
Compare
0620f3d to
8b0cd8b
Compare
|
Not a very technical review, but this looks pretty good to me, and I'm excited to see some design work around APIs (getting one step closer to emacs
|
08aec62 to
d4d49f3
Compare
Looks like we are missing highlighting for
The whole RFC is a rationalization and disentanglement of our current approach. Beyond the suggested "scripting" capability no new functionality will be added. The change should be invisible to the user. While I don't disagree that making a copy on migration would be better in a lot of ways (some downsides too), proposing that change in this RFC feels like the wrong place. Ideally we can adopt this API and then make changes like that after that, reflecting them both in the API and in our frontend. |
epatters
left a comment
There was a problem hiding this comment.
Thanks Kaspar, this is an excellent RFC, both in the content itself and in the clear and thorough presentation. I've got a few small questions/comments below, but, assuming I've understood things correctly, we should be able to move rapidly toward merging.
| const notebook = await binder.createNotebook(PetriNet, { name: "Example Petri-net" }); | ||
| ``` | ||
|
|
||
| We can dump a notebook. |
There was a problem hiding this comment.
I'm assuming that dump means "dump to JSON" and comes with the usual caveats about losing rich text annotations. Since this API abstracts over whether Automerge is being used, I don't see an alternative. I suppose we just have to accept that the abstraction is a little leaky.
There was a problem hiding this comment.
Yes, where "JSON" is a JS object that can be serialized to JSON. At least our existing JSON import / export has the same problems so we can justify it for dump and loadNotebook.
For interfacing to the editor I added a explicit leak in the form of getRichTextRef and cell.editorRef.
Thinking about it more though, programmatic edits being able to clobber the rich text cells is really not good, we should probably do something about that. One abstraction I had though of is to convert from and into markdown for this API but I haven't looked at the feasibility.
notebook.add(RichText, {markdown: "# Title"});There was a problem hiding this comment.
Conversion from/to Markdown seems worth exploring, in particular for having LLMs generate rich text. I'm happy to defer this to future work though if it seems like a lot of extra work.
|
|
||
| We provide some utilities for defining a logic: `defineMorphism`, | ||
| `defineObject` and `defineShape`. Here is what the definition of the `PetriNet` | ||
| logic looks like: |
There was a problem hiding this comment.
Let me check that I understand what's going on.
Currently, we define a logic/theory in the frontend by passing one big blog of JSON to TheoryLibrary.add. You replacing that API with a more granular one involving helpers defineObject, defineMorphism, and defineShape. This has the advantage that you can create side constants along the way like Place and Transition, which form part of the custom logic for this API. And that resolves my worry about previous designs that adding a new logic was already enough work and we shouldn't need a new, separate chunk of code to define the TS API: in this approach, you write the same amount of TS and get the API for free.
Is that right? If so, I think that's wonderful. I also think you should expand the text in the RFC a bit to clarify those implications of the design.
There was a problem hiding this comment.
I didn't realize I hadn't made that clear before! It is sort of fundamental to the whole approach that I actually don't even know what to say about it, which might be part of the problem.
There was a problem hiding this comment.
Ok, glad I'm understanding correctly. With that in mind, here's what I suggest to improve the exposition.
It seems that the idea of a "shape" is key to the whole design. However, almost nothing about shapes is said until Sec 2.14, which is about 2/3rds of the way through a long document. Moreover, the introductory part of the RFC (Sec 1) is very short, just a few sentences before launching into the technical details (Sec 2). I'd suggest:
- Rename Sec 1 from "Summary and motivation" to "Motivation"
- You could also expand the terse remark "...has led to entanglement of UI framework concerns with the modeling API" to clarify that we haven't had any API for document manipulation (outside the type defs), leading the UI components to perform lots of direct JSON manipulation
- Add a new Sec 2 called "Technical approach" with a couple paragraphs about the high-level approach (shapes), the pitfalls it avoids (requiring more boilerplate to manually define APIs for each logic), and the advantages it confers (e.g., type-safe manipulation of documents with varying degrees of type narrowing). The aim is to give the reader some idea of the big picture before launching into the details.
- Move current Sec 2 to Sec 3 and call it "Technical specification"
| ); | ||
| } | ||
| return thSymMonoidalCategory; | ||
| } |
There was a problem hiding this comment.
The helper getCachedTheory seems a slightly annoying piece of boilerplate to include for each analysis. Can this be avoided?
There was a problem hiding this comment.
The reason I did it this way as opposed to caching "in the document store" is that there doesn't seem to be a consistent thing to cache for the analyses (unlike for elaboration and validation of the model notebooks -- which we do cache in the document store). An analysis could in principle want to load and cache anything so it seems like the responsibility of the analysis implementer to take care of caching.
It feels like there could be a better design here but I haven't thought of it.
|
I added a section on "Validated models" to the "Future extensions". |
https://branch-kb-rfc-0006--catcolab.netlify.app/rfc/0006