Ex_doc: Makedown (and Makeup) + ExDoc

[tmbb]

Moved discussion here because it has nothing to do with ecto

With your PR to makedown, makedown itself depends only on ex_spirit, which obviously I can't stop depending on. Makedown will start depending only on:

• makeup_elixir (which depends on makeup and ex_spirit) and
• html_entities, which I can remove by adding your 6 lines from makeup
• earmark, which ex_doc already depends on.

This is as minimal as it will get, I guess. We can't drop the ex_spirit dependency, because ex_spirit is the whole reason makeup got anywhere. And the code is actually surprisingly small for something with so many features.

From the package owners's point of view, this isn't optimal, though. It requires adding makedown and makeup_elixir to the dependencies. Originally I made it so that makedown could be used independently of any particular markdown makeup_* module (of which there are only two right now, but hopefully more in the future). But let's be real: probably 99% of everyone who will use makedown in the future will do it with ex_doc, so maybe I should bundle makeup_elixir together with makedown? I'm not sure about it... It would save only a single line of config code. Most of the boilerplate is spent configuring ex_doc so that it can find the CSS and JS necessary for makedown to highlight things correctly. I don't think I can set those options from inside makeup, can I? I'm still a little unfamiliar with some aspects of Mix?

[josevalim]

You should probably open up an issue on Makeup because this discussion does not belong to ExDoc either. :D

In any case, I would like to highlight that the issue is not with the number of dependencies per se. If those dependencies are necessary, then they are necessary. It is still healthy to minimize them as much as possible because dependencies are a liability. They are more code to maintain, to fetch, to compile, to debug and may even provide more security vectors as well.

The only question I have is why we need to depend on both makedown and makeup_elixir. Is the latter an optional extension module that makedown depends on?

And my last concern is that the libraries have no tests. This can generate a lot of work on the ExDoc side because bugs will most likely be reported to ExDoc. If there are no guarantees that bugs can be fixed without introducing regressions, we can enter an infinite loop where bugs are being fixed but new bugs are always added.

[josevalim]

One last thing, makeup and makedown are really appreciated :heart:. Having those in Elixir is definitely better in the long term than having those in javascript.

[josevalim]

Most of the boilerplate is spent configuring ex_doc so that it can find the CSS and JS necessary for makedown to highlight things correctly. I don't think I can set those options from inside makeup, can I? I'm still a little unfamiliar with some aspects of Mix?

One of the things we could is to allow what goes before and what goes after to be specified in the markdown engine. In a way, highlight.js is specific to the markdown engine, so it makes sense to move the logic there. So we could add javascript_asset/0 and css_assets/0 to the engine which must return :none or {filename, contents}, such as {"highlight.js", blob_with_the_contents}.

We could use this opportunity to even decouple highlight.js from the JS that is used by ExDoc.

[josevalim]

@milmazz does EPUB work fine with JS? Does it also just uses highlight.js?

[tmbb]

You should probably open up an issue on Makeup because this discussion does not belong to ExDoc either. :D

My point is that it kinda does, because I don's see people using makedown any time soon except together with ex_doc...

The only question I have is why we need to depend on both makedown and makeup_elixir

makedown is a markdown processor. It depends on makeup.

makeup is a syntax highlighting library, in the spirit of Pygments. I've refactored Makeup to provide only the formatter (the part that converts the lexed tokens into HTML) and the styles (they are mostly CSS stylesheets, but I intend to make them something more in the future). The actual lexers are in separate packages (currently only makeup_elixir and makeup_html - makeup_css is on its way). I split the lexers because I use ExSpirit as a base. ExSpirit is veeery fast, but it compiles veeery slowly. @OvermindDL1 is supposedly working on making it compile faster in dev, but it will probably continue to be slow in prod. It's common to have a parser that takes >60-120s to compile. If I have n lexers included in the package, it can take more than 2n minutes to compile, which is not sustainable...

So makeup and makeup_elixir must remain in separate packages. The package makedown depends on makeup. If you want to use it to highlight a file with only HTML code blocks, you'll need only to import makeup_html5, and will have no use for makeup_elixir. This saves you some time when compiling your application.

But here we get to the point of me opening the issue here: if makedown is to be "coupled" to ex_doc, I can include makeup_elixir as a dependency. Otherwise, if it's meant to be decoupled, I shoudn't include it.

[tmbb]

We could use this opportunity to even decouple highlight.js from the JS that is used by ExDoc.

I think I've suggested it once. I tried to decouple it myself, but I could never build the assets on Windows, and I don't have a Linux machin on which to test it currently (working in a VM gets old fast).

On the other hand, keep in mind that highlight.js is still useful as second-line highlighter for languages other than elixir and HTML (the only languages makeup supports so far).

[tmbb]

So we could add javascript_asset/0 and css_assets/0 to the engine which must return :none or {filename, contents}, such as {"highlight.js", blob_with_the_contents}

You can get even more aggressive, like you've done with gettext. You could have something like an agent collect all resources at compile time, and render them to the file output. You probably wouldn't get a deterministic order, but there are ways around it, like resource names tagged with priorities and such.

[tmbb]

And my last concern is that the libraries have no tests.

Indeed. They are hard to test in a meaningful way, as they depend too much on visual inspection. Sure, I can litter the source with text cases (and I'll do it, I haven't done it because it's boring and I haven't had much time), and there are some obvious properties to text using something like StreamData, but so far, all bugs I've found were found through visual inspection of the HTML output and it wouldn't have occurred me to test them hadn't I seen the buggy output first.

And to make things worse, the output of makeup is actually non-deterministic, which will need some complications for unit testing (I explain at the end of the comment).

TD;LR: I'll add tests. I haven't added them yet because the important tests are boring, and the non-boring tests are not important.

The only part that is non-deterministic is the matching delimiters. I need to tag each pair of delimiters with a unique HTML attribute (so that the JS engine knows which pair to highlight on mouseover). I'm doing this using :erlang.unique_integer/1. Why can't I just keep a running counter (even the process dictionary), which would make it deterministic? Because the same webpage might have several code blocks which are independently rendered with makeup, and as such I need to guarantee that each pair of delimiters is tagged with a unique value for that particular BEAM run (this is not foolproof, because the integers might repeat if the BEAM is restarted, but that's as far as I'm willing to go).

[josevalim]

You can get even more aggressive, like you've done with gettext. You could have something like an agent collect all resources at compile time, and render them to the file output. You probably wouldn't get a deterministic order, but there are ways around it, like resource names tagged with priorities and such.

The problem gettext is trying to solve much more complex than this. Is there any reason why we would choose this more complex approach over the contract earlier proposed?

[josevalim]

But here we get to the point of me opening the issue here: if makedown is to be "coupled" to ex_doc, I can include makeup_elixir as a dependency. Otherwise, if it's meant to be decoupled, I shoudn't include it.

I think it would be better for makeup in the longer term to not be coupled to ExDoc nor to makeup_elixir.

[tmbb]

Is there any reason why we would choose this more complex approach over the contract earlier proposed?

Absolutely not. I was just saying it's possible, not that it was a good idea... This is the kind of thing you might have in the template engine of a web framework (there's a python framework doing something like this), not in a documentation generator.

[tmbb]

I think it would be better for makeup in the longer term

You're talking about makedown, right (sorry for the confusing names)? makeup itself will never be couple to anything, of course. It has uses beyond generating documentation (forums, blog engines, etc).

[josevalim]

Yes, I was talking about makedown. I think the names are actually nice, the confusion was on my part.

[josevalim]

@tmbb anyway, if you don't want users to specify to before tags and after tags, I would love a PR that adds javascript_assets and css_assets to the markdown engines and it can return a list of {basenames, contents}.

[tmbb]

I think the names are actually nice

It's all part of a long con with the goal of being able to write a "makeup tutorial" with a straight face.

One last thing, makeup and makedown are really appreciated ❤️. Having those in Elixir is definitely better than having those in javascript in the long term.

Thanks :) I wasn't even thinking forward when I wrote it... You don't even have to wait for the long term. For example, compare these two:

• original elixir docs using highlight.js
• my bootleg copy highlighted with makeup

There's a visible error in the 5th line with highlight.js. Struct literals have problems too in many cases (not always, and I have no idea what the problem is).

You could swap highlight.js with makeup tomorrow and the world would be better for it xD

[tmbb]

if you don't want users to specify to before tags and after tags, I would love a PR that adds javascript_assets and css_assets to the markdown engines and it can return a list of {basenames, contents}

I like this idea, but I don't think we should arbitrarily separate the assets into javascript and CSS. It should be before_closing_head_tag and before_closing_head_tag. People might want to put javascript above or below; that's not something ExDoc should decide.

And the user should also have the possibility of adding their own CSS and JS. I'm thinking of the following API.

User facing (configured in mix.exs):

• assets
• before_closing_head_tag
• before_closing_body_tag

In the markdown module:

• def assets(format), do: [{basename1, contents1}, {basename2, contents2}, ...]
• def before_closing_head_tag(format), do: ~S(<link rel="stylesheet" ...>)
• def before_closing_body_tag(format), do: ~S(<script ...></script>)

Things specified by the user would come after those specified by the markdown implementation so that the user can override them easily.

This API handles the default case with no configuration, and allows the user to add their own assets without interfering with the assets specified by the markdown implementation. The only think that can cause problems are name collisions. Solving name collisions here is a hard problem, and I'd prefer to raise an error on name collisions so that the user would have to rename their files in a way that didn't clash with the markdown assets (trying to handle them automatically brings you into a world of pain in which you can't specify arbitrary text in the before_closing_*_tag)

[josevalim]

@tmbb I like to start with the minimum API that works so starting with javascript+css as proposed instead of something more complex is good enough for me unless @milmazz disagrees because of epub concerns.

[tmbb]

Ok, but it's just that my solution is only a little bit more complex and way more general. I won't be able to dedicate much time to this right now, but I'll think about it an sketch an implementation.

EDIT: It's not really more complex, I guess it just moves the complexity around a little.

[josevalim]

@tmbb sure! Any of the APIs will be fine by me then. :)

[josevalim]

Shall we close this issue? Once the markdown callbacks are merged in, we can release a new ExDoc version with the cleaner usage for makedown.

[tmbb]

Yes. Maybe open an issue called "Add Markdown engine callbacks"? People there could pitch in with implementation ideas and maybe PRs

[milmazz]

does EPUB work fine with JS? Does it also just uses highlight.js?

@josevalim Sorry for the late response, but yes, EPUB works fine with JS and we're using highlight.js. But, we can use another module or alternative if you want. For example, we can ditch all the JS in our EPUB version, which right now in only loading highlight.js btw, and improve our CSS for the result after the makeup processing.