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

d2js: Additional render options #2343

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

x-delfino
Copy link
Contributor

PR to add support for the following render options in the wasm module and d2js:

  • Pad
  • Center: I'm not exactly sure how this is supposed to be used so not sure how to test it but it does add preserveAspectRatio="xMidYMid meet"
  • Dark Theme ID
  • Scale

Fixes #2307
Fixes #2297 (partially). I haven't added:

  • Fonts
  • Force Appendix
  • Bundle

Force appendix is probably easy, not really sure how bundle works atm and I'm not sure the best way to approach fonts - happy to take more of a look at this though.

There is a bit of duplication in the d2wasm functions atm (compile and render) - does that need consolidating?

Neither scale nor dark theme ID currently support -1 to use default values like the CLI does (rather no value/null should be passed for that option to use the default). does this need to be updated to match CLI behaviour?

screenshot of the customizable example

Screenshot

Thanks for the recent work on d2js, I've got it working in obsidian on iPad which is pretty sweet

@x-delfino
Copy link
Contributor Author

x-delfino commented Feb 11, 2025

ah I need to sort out the signing on those commits fixed

@x-delfino
Copy link
Contributor Author

I realised this doesn't actually fix #2307 - that issue was specifically referring to configuration variables (e.g. vars:) rather than just d2js supporting those options. will look into vars support but probably a bit more complex

@x-delfino
Copy link
Contributor Author

I think force-appendix and bundle are handled separately by the cli:

d2/d2cli/main.go

Lines 901 to 908 in 73f2588

if bundle {
var bundleErr2 error
svg, bundleErr2 = imgbundler.BundleRemote(ctx, l, svg, cacheImages)
bundleErr = multierr.Combine(bundleErr, bundleErr2)
}
if forceAppendix && !toPNG {
svg = appendix.Append(diagram, renderOpts, ruler, svg)
}

So defo a bit more work to get them supported

I'm not really sure what's going on for d2-config vars. only sketch: true seems to make a difference but it only partially sets sketch mode (changes the font):

sketch: false:

Screenshot

sketch: true:

Screenshot

expected:
Screenshot

@stereobooster
Copy link

stereobooster commented Feb 12, 2025

Thanks for the PR.

Just a small note (this is not related to the PR, it is for the context, for whoever will do next PR for bundle option). If you use generated SVGs as

<img src="/path/to.svg" width="..." height="..." />

Images inside SVG won't work, unless you use bundle option. Because

External resources (e.g. images, stylesheets) cannot be loaded, though they can be used if inlined through data: URLs.

SVG as an Image Restrictions

Copy link
Collaborator

@alixander alixander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool lgtm just some minor comments! Thank you!

P.s. would love to see that Obsidian plugin PR swapping D2 usage to d2.js

@x-delfino
Copy link
Contributor Author

Great - have made those changes now

I realised I haven't updated the defaults in https://github.com/x-delfino/d2/blob/4ce43c324831ff11411362382a41c74694ca18f4/d2js/js/src/index.js#L3-L6

is there much point in setting the additional default options in this wrapper library or just leave it to the d2 defaults?

and yep will get a pr raised for obsidian plugin later - just need to get it down a bit from 80MB...

@x-delfino x-delfino requested a review from alixander February 13, 2025 12:27
Copy link
Collaborator

@alixander alixander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for some reason scale doesn't have effect after a certain point. Not a big deal, probably just some max set by css, just noting it here, not a blocker.

scale.mp4

Playing around with it had 2 other small change requests. Sorry wasn't bunched with the first batch, but should be good to go after this.

@alixander
Copy link
Collaborator

alixander commented Feb 13, 2025

is there much point in setting the additional default options in this wrapper library or just leave it to the d2 defaults?

All good, just leave as is to d2.

and yep will get a pr raised for obsidian plugin later - just need to get it down a bit from 80MB...

is that a blocker? I.e. does obsidian have a max plugin size? I'm guessing most of that is D2 wasm size

@x-delfino
Copy link
Contributor Author

x-delfino commented Feb 13, 2025

don't think size is a blocker just would be nice to get it down a bit and the wasm module is only ~24MB atm. Also I'm thinking it makes sense to support both running the cli and wasm until they're at feature parity remembered that TALA isn't supported so we'll need to keep cli support - are there plans to support it in the wasm module?

@x-delfino
Copy link
Contributor Author

I've added in forceAppendix support as well, hope it's okay that I've squeezed that into this PR

screenshot

bundle is a bit more complex, I'm not sure of the limitations on WASM but it's failing to download remote images

@x-delfino
Copy link
Contributor Author

for some reason scale doesn't have effect after a certain point. Not a big deal, probably just some max set by css, just noting it here, not a blocker.
scale.mp4

Playing around with it had 2 other small change requests. Sorry wasn't bunched with the first batch, but should be good to go after this.

i think that's set by max-height here:

#output svg {
max-width: 100%;
max-height: 90vh;
}

I'm assuming that was set for the default scaling to fit

@alixander
Copy link
Collaborator

bundle is a bit more complex, I'm not sure of the limitations on WASM but it's failing to download remote images

ohh right, yes, bundle is not valid then for d2js client-side, though it can still be used d2js in node.

@x-delfino
Copy link
Contributor Author

missed a null check on the forceAppendix which broke things. fixed that now and added a test for force appendix

also I've been digging into some of the d2-config bits and worked out what's going on. The compiler merges in any render options from the source and then sets the defaults:

d2/d2lib/d2.go

Lines 72 to 73 in 41711d4

applyConfigs(config, compileOpts, renderOpts)
applyDefaults(compileOpts, renderOpts)

And the CLI just reuses that same object for the render so has all the correct properties set (including those set in d2-config)

So we just need to return an updated render options object from compile that can be passed back to render. I've got this working on another branch. would you want this as a separate PR or in here? it's a bit more of a change but don't think there's anything breaking. probably needs a bit of a cleanup first

One key thing to getting d2-config working is stripping the defaults from this library and just leaving them to be set by the compiler as d2-config wont be set if the corresponding option is already set

Screenshot

(this removes the need for the blank options in the example UI hence me not updating that here yet)

@alixander
Copy link
Collaborator

alixander commented Feb 14, 2025

remembered that TALA isn't supported so we'll need to keep cli support - are there plans to support it in the wasm module?

I haven't quite decided how that'll work. Since it's client-side, a license key wouldn't really work. I think it would still rely on system calls, but unsure. I think you should proceed ignoring it.

@alixander
Copy link
Collaborator

would you want this as a separate PR or in here?

Just throw in here

@alixander
Copy link
Collaborator

(this removes the need for the blank options in the example UI hence me not updating that here yet)

That looks 💯 !

@x-delfino
Copy link
Contributor Author

x-delfino commented Feb 20, 2025

alright, I've added also support for target, animateInterval, salt and noXMLTag. It does only support a single svg output still though (rather than a separate svg per layer/scenario/step)

Screen.Recording.2025-02-20.at.20.56.33.mov

I mostly copied the logic from the CLI with the exclusion of some relinking stuff. not sure exactly what that does or how best to approach that here

as far as I can tell the main missing functionality now is:

  • bundling
  • fonts
  • outputting different formats

I'm going to have a bit of a look at them to see how much work it would be. happy to either raise those as separate PRs or keep them here if I am successful

@MichaelCropper
Copy link

Would be awesome to keep PRs as small as practically possible if there is no direct dependency. Lots of great work already gone into this one so would be good to see it available for use :-)

@x-delfino
Copy link
Contributor Author

sure thing, I'll keep the other bits for separate PRs so this one doesn't get held up

I'm a bit confused on the failing CI task. It was successful on the prior commit https://github.com/terrastruct/d2/actions/runs/13444494745/job/37566593148. all I changed since then was some css in the d2js example

the tests are running successfully locally, including the specific task that failed here. I don't think the d2js changes should be relevant to this test anyway

> go test oss.terrastruct.com/d2/e2etests-cli
ok      oss.terrastruct.com/d2/e2etests-cli     11.420s

@x-delfino
Copy link
Contributor Author

hey @alixander - I'm not sure why the CI is failing here, all the tests are running fine locally. the prior run was successful and all I changed since then was CSS in the example webpage. any ideas? other than that, could you take a look at the recent changes please?

have been working on bundling and fonts but will raise another PR for them when they're ready

@x-delfino
Copy link
Contributor Author

x-delfino commented Feb 24, 2025

sorry noticed there was an error on the conditional elk generation for the browser worker

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

Successfully merging this pull request may close these issues.

D2JS - theme/center/pad has no effect on an svg d2js: full options args
4 participants