landmaj / mkdocs-d2-plugin

A plugin for embedding D2 diagrams in MkDocs.
https://landmaj.github.io/mkdocs-d2-plugin/
MIT License
25 stars 4 forks source link

Handle multiboard diagrams #4

Closed landmaj closed 11 months ago

landmaj commented 1 year ago

Composition is currently not supported due to D2 limitiation:

err: failed to compile test.d2: multiboard output cannot be written to stdout

There is this comment in D2 source code but no related issue, so chances are this will not be fixed in a near future.

It doesn't allow outputting to stdout because such diagrams produce multiple files. I can either fix this in D2 or figure out how to embed such diagrams in HTML and use a temporary file for rendering.

Karreg commented 1 year ago

I see 2 use cases that can be handled differently:

I find the second use case more interesting. The only other way to do that, is to duplicate the diagram, just to display a bit more information. and it breaks the only one golden source rule.

Karreg commented 1 year ago

I have done further tests after having a look at the source code of d2.

Here, a bit further, it looks like the scenarios and layers are correctly handled.

I have tried with a simple d2 diagram, and indeed all layers are created.

Source code:

direction: right
🧔‍♂️ -> 🌍: 👋

scenarios: {
    wave_back_scenario: {
        🌍 -> 🧔‍♂️: 👋
    }
}

layers: {
    wave_back_layer: {
        🌍 -> 🧔‍♂️: 👋
    }
}

with the result:

gitpod@6a7754c6b56c$ d2 docs/multi.d2 docs/multi.svg
success: successfully compiled docs/multi.d2 to docs/multi/layers/wave_back_layer.svg in 295.442628ms
success: successfully compiled docs/multi.d2 to docs/multi/scenarios/wave_back_scenario.svg in 293.367674ms
success: successfully compiled docs/multi.d2 to docs/multi/index.svg in 298.922065ms

When using with the plugin though, it generates errors:

gitpod@6a7754c6b56c$ mkdocs build
INFO    -  mkdocs-d2-plugin: Using cache at .cache/plugin/d2/db (dbm.ndbm)
INFO    -  Cleaning site directory
INFO    -  Building documentation to directory: /workspaces/docker-mkdocs/public
INFO    -  mkdocs-d2-plugin: source not found in cache.
ERROR   -  mkdocs-d2-plugin:
INFO    -  mkdocs-d2-plugin: source not found in cache.
ERROR   -  mkdocs-d2-plugin:
INFO    -  Documentation built in 8.31 seconds
Karreg commented 1 year ago

It would not be enough to get this feature to work, but the first step would be to stop generating diagrams to stdout in this plugin, and generate it to file directly. At least it would not get the multiboard output cannot be written to stdout error.

As a second step, it would require to analyze the output, and include the correct file, depending on the target passed as parameter of the include directive.

![](diagram.d2)

would result as including the diagram/index.svg file.

![](diagram.d2){scenario="wave_back_scenario"}

would result as including the diagram/scenarios/wave_back_scenario.svg file.

![](diagram.d2){layer="wave_back_layer"}

would result as including the diagram/layers/wave_back_layer.svg file.

What do you think?

Karreg commented 1 year ago

For the reference, an issue somewhat related to this one, on D2 side: https://github.com/terrastruct/d2/issues/1707

landmaj commented 1 year ago

If it can be handled on D2 side, I would strongly prefer that. This plugin is supposed to be simple and easy to pick up if you are already familiar with D2. There are 4 plugin-specific config options which do not affect rendering (executable, cache, cache_dir and render). Everything else is a 1:1 mapping to D2 command line flags, so if you want to know what an option does, just run d2 --help or search docs. Documentation of this plugin does not need to go into details about anything D2-related and the plugin itself does not introduce any new concepts. I would like it to stay that way.

What is more, D2 does not have a stable release and changes constantly. The plugin has no control over which version of D2 a user has. As a result, analyzing D2 output is a non-goal for this project as it could break at any point. I am considering removing hard-coded D2 options in favor of passing user-entered data as is to D2, allowing users to pass anything their version of D2 offers. For now the plugin only supports options I deemed useful, which is not ideal.

I recently implemented a feature and fixed a bug in D2, so I will look into the issue you linked, I may be able to implement it. No promises though, as it requires my free time and interest.

Karreg commented 1 year ago

Hi,

Just to clarify, since I think I have been misunderstood : I am not requesting to implement something in this plugin that is not handled by d2. That's why I'm referencing the issue on d2 side, just to keep an eye on it.

I get it about the stability of d2. what you are implying is that the implementation that could be done in the plugin, is that you fear that the current behavior of d2 may change over time so this feature will break. correct?

in this case, we can wait for the issue on d2 side, hoping it will work with stdout too, which is used by the plugin if I understand well the error message.

as of today, the plugin is really good already, and we can't ask it to be better than d2. And thanks for that!

landmaj commented 1 year ago

The issue with implementing this on the plugin side is that the plugin has no idea what to expect from the output. If you create a diagram with scenarios or layers, the output is a directory with, from the perspective of the plugin, randomly named files and directories. If you had nested layers, the output would be several directories deep. The plugin would have to parse your input, figure out the entire path and hope the file exists. It's a lot of complexity for something that would be deprecated (or removed) as soon as it is implemented in D2.

When it gets implemented in D2, the output would be a single file so it should work just fine with stdout.

landmaj commented 1 year ago

I opened a PR in D2 repo. When/if it gets merged, I will add support for --board flag in the plugin. https://github.com/terrastruct/d2/pull/1725

Karreg commented 1 year ago

Great job, thanks a lot!

Karreg commented 11 months ago

Hello,

The improvement has been done on d2 side. I have been working on a MR for your plugin so it can handle the flag, but as it looks like, the plugin is using ENV VAR to pass configuration:

result = subprocess.run(
            [
                executable,
                "-",
                "-",
            ],
            env={**os.environ, **env},

...

def env(self) -> Dict[str, str]:
        return {
            "D2_LAYOUT": self.layout,
            "D2_THEME": str(self.theme),
            "D2_DARK_THEME": str(self.dark_theme),
            "D2_SKETCH": "true" if self.sketch else "false",
            "D2_PAD": str(self.pad),
            "SCALE": str(self.scale),
            "D2_FORCE_APPENDIX": "true" if self.force_appendix else "false",
        }

However, they have not associated any ENV VAR to this flag:

    scaleFlag, err := ms.Opts.Float64("SCALE", "scale", "", -1, "scale the output. E.g., 0.5 to halve the default size. Default -1 means that SVG's will fit to screen and all others will use their default render size. Setting to 1 turns off SVG fitting to screen.")

...

    targetFlag := ms.Opts.String("", "target", "", "*", "target board to render. Pass an empty string to target root board. If target ends with '*', it will be rendered with all of its scenarios, steps, and layers. Otherwise, only the target board will be rendered. E.g. --target='' to render root board only or --target='layers.x.*' to render layer 'x' with all of its children.")

https://github.com/terrastruct/d2/blob/fa98423492fd9ee26e9be86a320f7777e0cb125b/d2cli/main.go#L120

To me, I first need to add an ENV VAR to d2 through an MR, and wait for its release, before I can propose an MR to you.

Is this correct or am I missing something?

Karreg commented 11 months ago

on a second thought, it makes sense not to add an environment variable, as the setting is local to the file, while other settings as environment variable can be used on all d2 files.

I see 2 options here:

WDYT?