Refactor NbBlock

[pietroppeter]

this is a major change, implements #168 (and also #117). Very WIP in a sandbox folder (see sandbox/notes.md)

[pietroppeter]

milestone reached, now in sandbox/minib3.nim both a minimal html and json backend work. (your json work was great @HugoGranstrom). Still lots of details to work on, but I am feeling more confident now!

A big realization was that in the end we might not need after all nim mustache anymore (we might still provide it for legacy reason and we should be able to provide a somewhat backward compatible NbLegacyDoc object that implements the default theme as before with partials), in a sense nimib itself is some code based (and block based) templating system with superpowers...

[pietroppeter]

(moved this note in sandbox/notes.md)

[HugoGranstrom]

Wow! Great work! 🥳 Looking good, it definitely seems way easier to define custom blocks now that the mustache backend is gone 😄 But I'm wondering how we will solve the problem of customizing an existing block type without it 🤔 But maybe that is what this idea is about?:

• can I also use a super method or something to wrap the rendered block in a NbBlock?
  • for example it could be used to add a class name inside a div
    • this could be important for example to customize code block appearance
  • or to add an optional id to a block
  • (this could anyway be added later)
  • and it could be added as something that by default a block does during rendering
  • it might have a different signature (takes rendering of content and outputs new rendering)

Mmh, no to customize an existing block the idea is to replace the function that renders it in the NbRenderobject here:

nimib/sandbox/minib3/minib.nim

Line 26 in 5e6727e

var nbToHtml: NbRender # since we need it for json, let's make it also for html

This sounds magical! Being able to modify the rendered code would be so cool and useful. The custom block code could stay very simple while we add all sort of features behind the scenes. Would we be able to do this with raw HTML though? Parse the HTML to and AST and modify the AST and then rewrite it as HTML?

The part above is still an idea and will need more fleshing out. I would definitely start with bare html and would allow customization only in the sense of: give me the rendered output and I can put things before or after. Any kind of refinement to that idea would need a use case that we think makes it worth it. And I would not probably push this during this refactor, maybe for now we do not even do what is sketched above (just to keep the scope manageable).

there is a tension between what you want to be able to serialize (content and specificities of a page) and what you want to be controlled (later) by the SSG (like the theme) and should not be serialized (also because it is redundant). Tension also with a third element which is the fact that rendering does need a theme

Is there really a tension? Couldn't we theoretically serialize the theme as well and just ignore/overwrite it in the SSG?

Yes, that is an option (serializing also the theme), but I guess it is kind of wasteful and also not "clean". I would like if possible to have a somewhat readable json. Especially if it is a low cost thing and a simple rule such as: everything that is in theme field of NbBlock is not serialized, the rest will.

Also, while we are refactoring the code, one part that has always confused me is nb.blk. I don't really see the point of it. Can't we just use nb.blocks[^1] instead (behind a template for easy access)? What value does adding it as its own variable (that you need to remember to set) bring?

Yeah, that is a good observation and it might not be needed after all. In principle you should not remember to set it since it should be in the NbBlock generating sugar, but if we can avoid needing it I could probably remove it. It will get trickier once I introduce real custom container blocks. My idea there at the moment is that NbDoc would have a add or similar field that represent what is used to add the block (the container could have multiple blocks fields, or maybe add, adds first to field first then to field second and that is it...). Then it could still be useful to have a reference to last block processed (which is needed for post processing, for the moment we have only the nbClearOutput in nimib, but I guess there could be other reasonable use cases).

Thanks for the feedback on the work-in-progress, it is useful :) nothing here is yet set in stone but I think a cleaner implementation is finally coming out.

[pietroppeter]

note, I just pushed a possible implementation of a NbContainer object (and NbDoc would inherit from this):

  NbContainer = ref object of NbBlock
    blocks: seq[NbBlock]
    parent: NbContainer

while the html backend works fine with it the json backend fails to serialize because of the circular reference. This is an issue with jsony that does not support this out of the box. It does not seem easy to fix. I tried to use skipHook (with a dev version of jsony), but since ref objects delegate to the non ref version, it does not work (I do not know if there is a way to get the type of the non ref version). I also tried with a custom dumpHook (which would not be nice to automate) but it also does not work. This issues should be probably better documented (maybe in a jsony issue?).

My current plan is to actually skip the parent field in NbContainer and have a containers: seq[NbContainer] field in Nb object (and add(nb: var Nb, blk: NbBlock) as a generic api that might support a different implementation.

[pietroppeter]

ok the new NbContainer implementation is indeed simpler, easier to implement and... it actually works! :)
I have also "hidden" the nb.blk api which is something that we might get rid of in the future.

Next steps:

• implement nbCode in minib
• implement nbCodeFromJs in minib
• think if there are essentially other types of blocks we need to test with the new refactoring
• if not go ahead and implement sugar to implement custom blocks (and refactor)
• clean up minib implementation
• reimplement all of nimib starting with the new minib poc

[HugoGranstrom]

Nice work! 🤩

I really like the withContainer and nb.add. Now that nb.blk is hidden, I don't have that much against it anymore tbh.

[pietroppeter]

notes from another discussion. in this PR we should also:

• reduce the number of global variables created (see the gotchas section of nimislides)
• have a non-global api where many blocks could be called without creating any globals (e.g. nb.text("hi"))

[HugoGranstrom]

So, I've started working on implementing nbJsFromCode. The part that I'm not sure about is where we should do the Nim->JS compilation. Should it happen before or after the transformation to JSON? It feels like it should happen before so that the JSON->HTML can happen without a Nim-compiler present + that steps become faster (important for SSG?).

Another thing that I'm wondering about is whether we should do the compilation the same way we are doing it now. Now we do it in nbSave: code
The thing is that we compile all of the nbJsFromCode blocks in the same file at the end, so we can't do the compilation before we have created all of them. I think the way we do it should be fine but I'm open to other ideas.

[HugoGranstrom]

The tests are finally fixed! 🥳
It seems like the test run for Nim 1.6 is failing for mysterious reasons, but stable is working!
And the preview deploy works as well! https://f0285381d1083ae4eeb2--nimib.netlify.app/

We are so close, now we just need to go through the code and clean it up. One thing I'm thinking about is moving all the block definitions out to other files. Maybe a folder with different blocks in different files? Maybe just a gigantic file with all the blocks? Do you have any preferences? The drawback of putting them in different files would be that we could stumble upon cyclic imports if we want to share stuff between them.

[pietroppeter]

I would probably go with single file with all blocks

[HugoGranstrom]

Great, then I'll fix that when I get some time over 👌

[HugoGranstrom]

That worked out way smoother than I expected. Now I just need to clean up some comments and then we should be ready for a final review. 🥳

[HugoGranstrom]

There was less to be done than I thought, so now the PR is officially ready for review @pietroppeter 😉

[pietroppeter]

Great will give it a go soon enough. You should mark it as non draft

[pietroppeter]

Note: This is a Claude-generated code review, produced in a Claude Code Web session.

Review: PR #235 — Refactor NbBlock

Summary of Changes

1. New wrapper type Nb (previously nb: NbDoc)

The old nb was an NbDoc value directly. Now nb is an Nb wrapper object containing nb.doc, nb.blk, nb.containers (stack for nested blocks), and nb.backend. This separation keeps document data (NbDoc) cleanly distinct from the build-time state needed to produce it (Nb) and enables the withContainer pattern.

Breaking change: code that accessed nb.source, nb.filename, nb.thisFile, nb.options, nb.cfg etc. directly now needs nb.doc.<field>. End-user notebooks using nbCode:, nbText: etc. are unaffected.

---

2. NbBlock becomes a polymorphic ref hierarchy

The old NbBlock had command, code, output, context as flat fields on every block. Now each block type is its own ref object of NbBlock with only the fields it needs. The kind: string field on the base type carries the type name for JSON dispatch. New types: NbCode, NbText, NbTextWithCode, NbImage, NbFile, NbVideo, NbAudio, NbDiv, NbRawHtml, NbJsFromCode, NbJsFromCodeOwnFile, NbPython.

---

3. Mustache dependency removed

requires "mustache >= ..." removed from nimib.nimble. Rendering is now pure Nim functions — type-safe, no external template language for theme authors to learn. (The docsdeps task still installs mustache@0.4.3 for example docs that use it directly — that's fine.)

---

4. New NbRender system (globals.nim, renders.nim, themes.nim)

nbToHtml and nbToMd are NbRender ref objects each holding:

• funcs: Table[string, NbRenderFunc] — per-block-kind render function keyed by type name
• partials: Table[string, NbPartialFunc] — named fragments (head, header, footer, nbCodeSource, etc.) that themes override

Themes configure the active backend by swapping partials and setting nb.doc.context JSON values.

---

5. newNbBlock macro and nimibSugars.nim

Eliminates boilerplate: one newNbBlock(NbFoo): field: type; toHtml: ... call auto-generates the ref object type, a newNbFoo(...) initializer, the nbFooToHtml proc, registration in nbToHtml.funcs, and addNbBlockToJson(NbFoo). The withNewlines macro handles the common pattern of building multi-line HTML strings conditionally.

---

6. All block definitions moved to builtinBlocks.nim

src/nimib.nim goes from ~300 lines to ~70 lines. All block definitions live in builtinBlocks.nim. JS/Python blocks live in jsutils.nim.

---

7. JSON serialization (jsons.nim)

Enables document interchange and is a prerequisite for nimibook. addNbBlockToJson registers parse/dump hooks for each block type via the kind string discriminator using jsony.

---

8. NbContainer and nested blocks

NbContainer adds blocks: seq[NbBlock]. nb.withContainer(blk): body manages the stack. blocksFlattened provides a flat traversal.

---

Items Worth Addressing

A. Dead code: newNbBlockOld in blocks.nim 🔴

Renamed from newNbBlock with a # do we need this anymore? comment but never removed. It still references newContext (mustache). Since mustache is no longer a dependency this will fail to compile if called. Should be deleted.

B. nbJsCounter and id are in NbDoc, not Nb 🟡

The comment in types.nim explicitly says # Nb should just contain stuff needed for producing the NbDoc (like id and nbJsCounter) — but both fields are still on NbDoc. These are build-time counters, not document content. Intent not implemented yet.

C. NbDoc.context is still a JsonNode grab-bag 🟡

Theme config values (stylesheet URL, title, highlight settings, github URL, path_to_root, source, etc.) all live in nb.doc.context: JsonNode. This is a deliberate compromise (typed fields for block content, JSON for theme/metadata) but the boundary is fuzzy. Worth a comment documenting the design decision.

D. Version number not bumped 🔴

.nimble still reads version = "0.3.13". The nb: NbDoc → nb: Nb change is a breaking API change for anyone accessing nb fields directly — this warrants at least 0.4.0.

E. No changelog entry 🟡

changelog.md has no entry for this refactor. At minimum the "Breaking changes" section should describe the wrapper change for advanced users.

F. NbJsFromCode should inherit from NbCode but doesn't 🟢

The comment in jsutils.nim: # should inherit from nbCode, but it is defined in nimib.nim.... Now that all built-in blocks are in builtinBlocks.nim, this circular dependency is gone. The inheritance could be done now.

G. withNewlines doesn't handle for loops 🟡

nimibSugars.nim: # TODO: handle for loops — withNewlines silently produces wrong output if the body contains a for loop. Could be surprising for theme authors writing custom renders.

H. NbFile render is not using partials 🟢

The comment in builtinBlocks.nim: # Make pre-code with hljs-extensions into a partial! — inconsistent with how NbCode renders (which does use partials). Resolve or remove the comment.

I. nbInit parameter renamed backend → renderer 🟢

Minor unnecessary churn for anyone using the named parameter form nbInit(backend = ...).

---

Overall

The refactoring is architecturally sound. The core changes (polymorphic blocks, mustache removal, newNbBlock macro, centralised builtinBlocks.nim) work together coherently and accomplish the goals described in #168 and #117.

Must-fix before merging: A (dead code referencing removed mustache dependency) and D (version bump). Items B, E, F, G, H are good candidates for follow-up issues.

[pietroppeter]

Tried to review with Claude. It does seem it has done a decent job, but now of course, I need to review the review :)

[HugoGranstrom]

B. nbJsCounter and id are in NbDoc, not Nb 🟡

I'm thinking a bit about this and it doesn't hurt to include these fields in the NbDoc instead of in Nb. For example if we want a feature in the future where you can append to an already existing NbDoc (loaded from a json file). Then it's good to know what the id counter is at to avoid collisions. So I vote for keeping it as is and just remove that comment. What do you think?

[HugoGranstrom]

F. NbJsFromCode should inherit from NbCode but doesn't 🟢

I tried fixing this, but we must have a bigger cycle of imports that I can't seem to find. So I'll keep it as it is for now