d2plugin: Sketch plugin #45
Conversation
alixander
force-pushed
the
alixander/sketch-plugin
branch
from
a2504ee
to
0f8ca81
Compare
alixander
force-pushed
the
alixander/sketch-plugin
branch
from
ef3e86c
to
e672577
Compare
| @@ -37,6 +37,7 @@ func run(ctx context.Context, ms *xmain.State) (err error) { | |||
| bundleFlag := ms.FlagSet.BoolP("bundle", "b", true, "bundle all assets and layers into the output svg") | |||
| versionFlag := ms.FlagSet.BoolP("version", "v", false, "get the version and check for updates") | |||
| debugFlag := ms.FlagSet.BoolP("debug", "d", false, "print debug logs") | |||
| sketchFlag := ms.FlagSet.BoolP("sketch", "s", false, "use the sketch plugin, making diagrams look sketched by hand") | |||
There was a problem hiding this comment.
what's the value in this being a plugin? Why not just handle it directly?
There was a problem hiding this comment.
Nvm, I understand the v8 dependency is nice to be optional.
There was a problem hiding this comment.
Should we add a $D2_RENDER to control which renderer plugin is used?
There was a problem hiding this comment.
i think it's different from layout, where you always need one to be present. i think a value-less flag is most appropriate here.
more broadly tho, i think we should either have equivalency of options across flag and env vars or go with only one. it's rly confusing remembering which are set as env vars and which are flags.
There was a problem hiding this comment.
For sure, see #53
What if we add another type of renderer plugin though?
I think --renderer=sketch is reasonable? And for switching back, --renderer=default. Perhaps -s can be a shorthand for --renderer=sketch.
There was a problem hiding this comment.
Not sure what you mean by second point? What are you referring to?
same thing as u said about it having order. okay, i think that's a strong case for grouping under 1 flag. should we call it render tho? then if other plugins do other things, we need separate flags. (e.g. idk, an xss plugin that escapes harmful labels). wdyt about just --plugins
There was a problem hiding this comment.
I would consider an xss plugin to be a render plugin. Though that should be something that's builtin and not a plugin.
The problem with --plugins is that now you can pass layout plugins too. What should that mean? What if you have a layout plugin after a render plugin?
There was a problem hiding this comment.
yeah ur right. okay, --render=x,y,z it is
There was a problem hiding this comment.
fyi @nhooyr, revisiting this, now that sketch is not a plugin (since we removed the v8go dep), it should be treated as a boolean flag because it's a replacement for the default render. Back when it was a plugin, we drew the default, and then targeted with regex to replace (which is like a pipeline). Now that it's not a plugin, we can just draw it as desired the first time.
The pipeline mechanism still makes sense to me for post-render steps, like the "high-contrast" or "colorblind" as discussed. But for sketch, I'm just going to make it a boolean flag.
There was a problem hiding this comment.
Sketch can still be made a plugin to avoid having to pull in an entire JS engine for builds only using TALA.
I think it's ok to make it a flag for now, makes it much easier to use.
| Info(context.Context) (*PluginInfo, error) | ||
|
|
||
| // Options returns permitted operations for the plugin. | ||
| // "layout", "postProcess" |
There was a problem hiding this comment.
why not return a struct where each option is a bool?
There was a problem hiding this comment.
yeah that'd be better
| return nil, err | ||
| } | ||
| js := `const root = { | ||
| ownerDocument: { |
| if _, err := v8ctx.RunScript(roughJS, "rough.js"); err != nil { | ||
| return nil, err | ||
| } | ||
| js := `const root = { |
There was a problem hiding this comment.
move const root onto its own line to see indent clearly.
Also implements being able to run D2 with multiple plugins, by adding a
Options()on d2plugin interface that returns the list of operations the plugin supports.