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.

[pietroppeter]

Looks good to me!
I like also the sugar.

Couple of things I am noticing:

• we are inheriting nbCode from NbContainer to allow for container blocks in nbCode, right? (so that we do not need anymore nimibCode)
• TIL about the braces syntax in JsonNode I guess the advantage is it does not raise KeyError, right?

[HugoGranstrom]

Nice! Then I'll go ahead and implement it for the other blocks as well 👍

we are inheriting nbCode from NbContainer to allow for container blocks in nbCode, right? (so that we do not need anymore nimibCode)

Exactly, nimibCode is basically an alias for nbCode now.

TIL about the braces syntax in JsonNode I guess the advantage is it does not raise KeyError, right?

That is correct. I also didn't know about it until a few weeks ago. It's really nice as it handles nil sielently so you can chain multiple of them without worrying:

var j: JsonNode = nil
j{"hello"}{"world"}.getStr("default string")

This will just work, j{"hello"}{"world"} evaluates to nil and .getStr("default string") checks if it is nil or of the wrong kind and then uses the default value. So it is already really ergonomic to work with JsonNodes :D

Edit: it can be written even shorter:

j{"hello", "world"}.getStr("default string")

[HugoGranstrom]

HugoGranstrom/nimiSlides#52
I think this PR shows that it is totally possible to rewrite a nimib library using this refactoring.

[HugoGranstrom]

I can't imagine nimibook being more complicated. It doesn't really define that many custom blocks, it's more of the tooling around it and a theme. So I would be okay with saying that this PR doesn't break anything for the other libraries. And that means (by looking at my todo list hidden in the middle of this thread) that I should be fixing a rewriting tests 🥳

[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. 🥳