Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove built-in plugins from plugin package #1752

Open
flowchartsman opened this issue Nov 30, 2023 · 8 comments · May be fixed by #1753
Open

Remove built-in plugins from plugin package #1752

flowchartsman opened this issue Nov 30, 2023 · 8 comments · May be fixed by #1753

Comments

@flowchartsman
Copy link

flowchartsman commented Nov 30, 2023

I am in the very early stages of building my own layout plugin for d2 and everything works mostly well, however I am forced to use build tags in order to exclude the built-in plugins. This is due to their built tags being negative instead of positive. It would probably be better to have the built-in plugins in a separate package, but changing the build tag system from //go:build !nodagre to //go:build bundledagre and then including these in your release scripting would also work. Willing to submit a PR, if needed.

@cyborg-ts cyborg-ts added this to D2 Nov 30, 2023
@alixander
Copy link
Collaborator

@flowchartsman Why do you want to exclude it? You would just do d2 --layout=flowchartsman-layout input.d2.

@flowchartsman
Copy link
Author

flowchartsman commented Dec 1, 2023

@alixander It's because I'm building a plugin, which makes it a binary that depends on oss.terrastruct.com/d2/d2plugin to provide the IPC interface to the main binary. d2plugin then includes both oss.terrastruct.com/d2/d2layouts/d2dagrelayout and oss.terrastruct.com/d2/d2layouts/d2elklayout unless build tags are used to exclude them. Both of these layout engines use the (admittedly very clever) technique of embedding their javascript dependencies and running them with goja, however, this means that you cannot write your own d2 plugins without pulling in both of these dependencies as well (and their embedded assets), unless you use the build tags to exclude them.

I can see why the decision was made to bundle dagre and elk into the package, but this hampers the use of d2plugin as an importable package, since every user will need to remember to use these build tags to avoid pulling in code they don't need.

❯ go build -tags nodagre,noelk -o myplugin ./cmd/d2plugin-gograph
❯ go build -o myplugin_withouttags ./cmd/d2plugin-gograph
❯ l❯ du -h myplugin*
 26M    myplugin
 31M    myplugin_withouttags

Thus, my suggestion would be to convert them to positive tags and add these to the release tooling or, better still, move them to separate packages completely and add a d2plugin.RegisterEmbeddedPlugin(p Plugin) method, that itself is gated behind and plugins_embed build tag.

Something like this:

/*d2/d2plugin/embedded.go*/
//go:build plugins_embed
package d2plugin

func RegisterEmbeddedPlugin(p Plugin) {
    plugins = append(plugins, p)
}

Then move the entire contents of d2/d2plugin/plugin_dagre.go and d2/d2plugin/plugin_elk.go into d2/internal/embeddedplugins/dagre/dagre.go and d2/internal/embeddedplugins/elk/elk.go, giving them the build tags

//go:build plugins_embed && plugins_embed_dagre

and

//go:build plugins_embed && plugins_embed_elk

Respectively, and modify them to call d2plugin.RegisterEmbeddedPlugin in init(), since they will then be outside of the d2plugin package and will need a way back in. Then the release process will need to add -tags plugins_embed,plugins_embed_dagre,plugins_embed_elk into the workflow, and that's it.

This way embedded plugins can still register themselves for production builds but d2plugin.RegisterEmbeddedPlugin will otherwise be unexposed to anyone importing d2plugin, unless they want to use the build tags to build their own custom version of d2 that embeds more plugins. This also provides more flexibility for the team, since they can choose to cut releases with and without specific plugins, should they desire.

@flowchartsman flowchartsman linked a pull request Dec 1, 2023 that will close this issue
@flowchartsman
Copy link
Author

Ended up being a bit more than that, since the plugin types were unexported, however moving them to internal and making them exported was enough to solve the issue. Now they are included as part of the process of building d2/d2cli. There are certainly other ways to organize this and you could also just use a single tag to embed all of the packaged "standard" plugins, but at least doing it this way d2plugin is now separate from any implementation.

@flowchartsman
Copy link
Author

flowchartsman commented Dec 2, 2023

Having considered this some more, I think you could get away without build tags entirely by moving all of the dagre/elk code inside of internal (since there's probably not much need for a user to include either of them), and just allowing RegisterEmbedded plugin to be exported. This would essentially be a no-op if used in a plugin anyway, but you could still gate it with a single embed_plugins build tag, if you wanted. I'll convert this PR to a draft for now and make those changes.

@alixander
Copy link
Collaborator

this means that you cannot write your own d2 plugins without pulling in both of these dependencies as well

Why is this so undesirable?

...to avoid pulling in code they don't need.

Is it just the extra size? Goja is pure go, so there's no concerns about dependencies causing anything else.

@flowchartsman
Copy link
Author

flowchartsman commented Dec 7, 2023

Why is this so undesirable?

It's undesirable because it's completely unnecessary, makes building plugins more difficult, and kind of goes against the otherwise-clean plugin abstraction you've set up. It feels like a minor oversight to compile two unrelated bundled layout plugins into every plugin someone might want to develop, and it's easy enough to fix by just moving the bundled plugin integration into the CLI, which is where it makes more sense to put it since it's the primary release artifact (this is what my second PR would be).

Is it just the extra size?

Not just that, it also made it difficult to build the plugin, since there was a mismatch between the xmain in HEAD and the xmain used by int the most recent release. I ran into some weird issues there where my build started throwing compilation errors from within the cached d2 module itself, and it took me awhile to figure out that I needed the build tags, and was an unpleasant development exercise. Especially since the fix is relatively straightforward.

The PR I'm currently working on simply moves the initialization of the bundled plugins into the CLI package which is probably where they belong anyway, since that's the primary release artifact. It also cleans up the abstraction and moves the bundled plugins into their own package. It's not the end of the world or anything, but it's definitely the cleanest way to handle it if you actually want people to be able to build their on plugins.

@alixander
Copy link
Collaborator

I see. Yeah I think that's right that they should be initialized by the CLI instead of a lib package. I'll take a look at the PR shortly.

@flowchartsman
Copy link
Author

flowchartsman commented Dec 7, 2023

Don't look at the current PR too seriously. I think it's more complicated than it needs to be and touches more of the build than it needs to. There is a more elegant way to do it that I will submit soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging a pull request may close this issue.

2 participants