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]

Regarding the partial/backend thing, this is definitely something I am noting myself down to try and help thinking out a solution. This is definitely a breaking change, but I think it should be worth it.

Not sure if it helps but the general idea in the past was: a backend is a way to compile to a different format (html vs md vs ...). Backend could support theming and overriding (but basically it made sense for html theme only.

Right now we kind of realized that maybe backend abstraction is less customizable than we thought (json backend needs to be baked in). And we could try and keep only a layer of customization at the theme level. Not necessarily it needs to work in the same way (i.e. we could have a way of overriding by inheriting...).

Still if, after thinking of what should be the best way to support theming, we figure out also a way to support the old way (mustache had the advantage that you could also change the theme with files that override partials) it would be nice.

These are my immediate thoughts and I might be missing a lot. Probably at some point it could be nice to have a lice conversation about this, but maybe in the meantime we could start an async one.

[HugoGranstrom]

Thanks for your thoughts 😄 Yes it is a bit of a complicated matter. I still think the backend+ theme is a good way to work. But we need to be a bit more precise in exactly what each part should be responsible for. They way we did it before the refactor was:

• The theme populated the context object with global variables and settings
• The backend handled all things rendering using the values in the context.
  So here the theme wasn't really about how the blocks were rendered (well some blocks did use context values but let's skip them for simplicity). It mostly controlled the CSS for theming.

One approach going forward I could see is moving the customizable partials to the theme as well. And the backend's responsibility is to render the block from those partials and hardcoded HTML. Having written this though, I don't really see the point in separating them. A theme will generally only work with one backend, so why split them up? Let the theme decide the backend. Or am I missing something here?

[HugoGranstrom]

Thinking about it a bit more, I see two kinds of customizations (using nimiSlides as reference)

1. Page-wide customization. Typically, the document layout and CSS (nimiSlides uses Reveal.js and need another page layout than base nimib).
2. Block-rendering-customizations. Typically, changing how a certain block looks. (nimiSlides adds classes to nbCode that Reveal.js needs)

The first kind of customization is currently possible by modifying nbToHtml["NbDoc"] = renderNimiSlidesDoc. The second kind of customization is also possible in the same way nbToHtml["NbCode"] = renderNimiSlidesNbCode. BUT I would have to do this for all NbCode-like blocks because there is no way to have reusable and exchangable parts in them. For example:

toHtml:
    withNewlines:
      if blk.code.len > 0:
        &"<pre><code class=\"nohighlight hljs nim\">{blk.code.highlightNim}</code></pre>"
      nbContainerToHtml(blk, nb)
      if blk.output.len > 0:
        &"<pre class=\"nb-output\">{blk.output}</pre>"

Even if I rewrote this to use functions for rendering the code and output, it wouldn't be possible to change it because the function is hardcoded. So I would have to rewrite the function altogether and replace the function calls with my own. So if instead I could do:

toHtml:
    withNewlines:
      nb.renderPartial(blk, "nbCodeSource")
      nbContainerToHtml(blk, nb)
      nb.renderPartial(blk, "nbCodeOutput")

Then I would be able to change the partials and it would change everywhere it is used (in all NbCode-like blocks). BUT the problem is that these two particular partials would need blk to be a child-type of NbCode which is hard to enforce. In this case it is fair to require it. But say that we have a more general partial that in theory would work with all types that has an output field. How would we handle that? One idea is to convert all blocks sent to a partial to JsonNode. That way we could pass all sorts of blocks to the partial and it would work.

One thing I've realized when writing this is that we aren't really using the inheritance a lot at all when rendering. We are using it for storage and creation. But it feel like we have untapped potential when it comes to rendering. I guess it is hard when you have different backends and want customization. Then we are basically stuck with the table-of-functions that we are using now. So maybe it isn't even desirable to use inherticance when rendering 🤔

This was a lot of rambling and I mostly wrote it down to not forget it hehe😅So no need to reply unless you found something useful.

[HugoGranstrom]

all docs are up! (except index.md because of lack of md backend) 🎊

[HugoGranstrom]

I've been thinking a bit more, and I think it boils down to that we want a system that is:

1. Customizable

• It should be possible to change how a block is rendered

2. Re-usable

• It should be possible to re-use components from other blocks. For example, I should be able to style my block like an nbCode block.
• BUT, it should ideally be possible without my block being a direct descendant of the block. Imagine I want to use parts from multiple different blocks, for example. My block can't inherit from both of them!

Our options thus far:

• Table[string, proc(blk: NbBlock, nb: Nb): string
  • Fulfills requirement 1 but not requirement 2
  • The problem stems from the fact that the NbBlock must be converted to the expected kind to access its fields. This only works for direct descendents of the block.
• Inheritance
  • Doesn't fulfill any of the requirements
• Regular functions
  • Fulfills requirement 2 but not requirement 1
• Table[string, proc(blk: JsonNode, nb: Nb): string
  • Fulfills both requirements
  • The fact that it accepts a JsonNode allows us to pass in different parameters to different procs
  • This would also allow us to pass in other parameters than the fields of the blocks. For example, we could pass in a list of classes to a block.
  • The downside is that we get fewer compile-time guarantees and have to do more manual checks to see if fields exist. But hopefully this can be dampened by sugar.

I may have missed some options, but currently I see embracing JsonNodes as our best bet. So a typical toHtml proc could look like:

func somePartial*(blk: JsonNode, nb: Nb): string =
    if not blk{"code"}.isNil:
      let classes = blk{"classes"}.getElems.mapIt(it.getStr).join(" ")
      &"<code class=\"{classes}\">" & blk{"code"}.getStr & "</code>"
    else:
      ""
nb.partials["somePartial"] = somePartial

func someBlockToHtml*(blk: NbBlock, nb: Nb): string =
  let blk = blk.SomeBlock
  var jObj = blk.toJson
  jObj["classes"] = %["highlightjs-nim", "reveal-js-code"]
  withNewlines:
    "<h1>" & blk.title & "<h1>"
    nb.renderPartial("somePartial", jObj)

It is more verbose, but I think we could make sugar to simplify the process quite a bit.

What do you think?

[HugoGranstrom]

The biggest problem with this, I would say is that it is hard to know what fields a particular partial function expects. I don't see how we could handle that at compile-time. But some sugar to check for required and optional fields at runtime should be possible. Then the writer of partials would have to write fewer checks manually at least.

[pietroppeter]

This is very interesting development, will have to think and reconnect with the actual code to give an informed opinion (current goal is to find time this weekend).

Just so I understand, right now we have been implementing inheritance part (and part of the complexity of serializing to json came from that), but according to what you are saying, it is not worth the effort and we should instead revert to something more similar to what we had (only going all in on Json Node)?

[pietroppeter]

First thought anyway would be to use a distinct JsonNode to control the api (but this of course is a minor point).

[HugoGranstrom]

I think we should keep inheritance (different field for different blocks are good and we get some kind of CT-safety). But for the rendering we don't have much use for it. So there we covert it to JsonNode. But going all in on distinct JsonNode is also an option I haven't considered :O

[pietroppeter]

Ah ok I was understanding wrong. :) Yeah, all in to JsonNode for rendering looks very reasonable for me.

[HugoGranstrom]

Nice to hear. I'll give a try implementing it today and post an example here so we can discuss if we like the syntax and what parts we would like sugarify 😄

[HugoGranstrom]

It was surprisingly easy to implement! And the best thing about this partials stuff is that it's totally optional. If you (a random nimib block creator) don't want to use partials, you can just go your merry way and don't even have to deal with JsonNodes.

Here's how we implement NbCode now:

func nbCodeSourcePartial*(blk: JsonNode, nb: Nb): string =
  let code = blk{"code"}.getStr
  if code.len > 0:
    &"<pre><code class=\"nohighlight hljs nim\">{code.highlightNim}</code></pre>"
  else:
    ""

nbToHtml.partials["nbCodeSource"] = nbCodeSourcePartial

func nbCodeOutputPartial*(blk: JsonNode, nb: Nb): string =
  let output = blk{"output"}.getStr
  if output.len > 0:
    &"<pre class=\"nb-output\">{output}</pre>"
  else:
    ""

nbToHtml.partials["nbCodeOutput"] = nbCodeOutputPartial

newNbBlock(NbCode of NbContainer):
  code: string
  output: string
  toHtml:
    withNewlines:
      nb.renderPartial("nbCodeSource", jsonutils.toJson(blk))
      nbContainerToHtml(blk, nb)
      nb.renderPartial("nbCodeOutput", jsonutils.toJson(blk))

It is (as you said above), basically our old mustache-workflow, but with JsonNode instead.

Here's a proposal for a sugar:

func nbCodeSource*(nb: Nb, text: string = defaultValue, code: string): string {.nimibPartial.} =
	text & code

# rewritten to:
func nbCodeSource*(nb: Nb, blk: JsonNode): string =
	let text = blk{"text"}.getStr(defaultValue)
	let code = blk{"text"}.getStr(default(string))
	text & code

It should be pretty straight forward to implement. It helps document the inputs as well as they are part of source code. The handling of the abscence of a certain fields could probably be improved. For example, if no default value is given, maybe we raise an exception as it's a mandatory field? Then the let code = ... line would first check that the key exists and is a string.

The problematic part is seq[T], Table[string, T], as it involves nesting. But it's not undoable so we'll handle it in due time.

[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")