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

Wavedrom support #89

Closed
JmyL opened this issue Jan 13, 2016 · 15 comments
Closed

Wavedrom support #89

JmyL opened this issue Jan 13, 2016 · 15 comments
Milestone

Comments

@JmyL
Copy link

JmyL commented Jan 13, 2016

It would be nice to have Wavedrom(https://github.com/drom/wavedrom) as an preprocessing extension.
It is 4 years old open-source software, It's really cool to adding waveform in documents.
It's implemented with javascript, but it also served as an executable cli version on here: https://github.com/wavedrom/wavedrom.github.io/releases

@pepijnve
Copy link
Member

Looks like a perfect fit for asciidoctor and the diagram extension.

I couldn't find any documentation on the CLI interface. Just as a note to self, based on the code, the way to invoke it is wavedrom <input file> <output type> <output file>.

pepijnve added a commit to pepijnve/asciidoctor-diagram that referenced this issue Jan 13, 2016
@pepijnve
Copy link
Member

@JmyL I've made an initial attempt at integrating wavdrom support using the CLI version. On Mac OS X it doesn't work correctly yet. The command line arguments that I pass to nw.js aren't reaching the application (argv is an empty array in wavedroms init.js). I tried directly calling the nwjs binary and starting the application using open WaveDromEditor.app, but both failed to produce the correct results. This seems to be a known issue (see wavedrom/wavedrom.github.io#7)

I have some unfinished code lying around that used phantom.js to run javascript code. I'll have a look at using that to drive the javascript code directly rather than using the nw.js wrapper.

@drom
Copy link

drom commented Jan 13, 2016

Proper CLI would be:

WaveDromEditor source <input file> <output type> <output file>

I have some old phantomjs version of CLI tool, but I was thinking that pure nodejs CLI tool would be better. Working on it now.

@pepijnve
Copy link
Member

Adding the missing 'source' keyword did the trick. Thanks!

pepijnve added a commit to pepijnve/asciidoctor-diagram that referenced this issue Jan 13, 2016
pepijnve added a commit to pepijnve/asciidoctor-diagram that referenced this issue Jan 13, 2016
pepijnve added a commit to pepijnve/asciidoctor-diagram that referenced this issue Jan 13, 2016
@drom
Copy link

drom commented Jan 13, 2016

@JmyL @pepijnve let me know how successful integration is 👍

pepijnve added a commit to pepijnve/asciidoctor-diagram that referenced this issue Jan 13, 2016
@pepijnve
Copy link
Member

The code is all there and the unit tests are passing. I manually inspected the PNG and SVG files that get generated during the tests and they correct to me. Once Travis gives the green light on Linux, I'll merge the feature branch and cut a new release.

pepijnve added a commit to pepijnve/asciidoctor-diagram that referenced this issue Jan 13, 2016
pepijnve added a commit that referenced this issue Jan 13, 2016
@pepijnve
Copy link
Member

I had to disable the unit tests in the end since wavedrom can't run headless at the moment. I've merged what I have already, but using phantom.js is probably the way forward after all since that's designed to run on servers.

@pepijnve pepijnve added this to the 1.3.2 milestone Jan 13, 2016
@JmyL
Copy link
Author

JmyL commented Jan 14, 2016

Incredible! 👍

@pepijnve
Copy link
Member

You're welcome. Let me know if you run into any issues with the integration. You'll probably be the first to kick the tires...

@mojavelinux
Copy link
Member

I second that. 👍

@drom
Copy link

drom commented Jan 15, 2016

@pepijnve Great job!
Will it help if I have pure nodejs module that does not require DOM, NW.JS or PhantomJS?

@pepijnve
Copy link
Member

@drom for the asciidoctor-diagram integration itself it doesn't really matter. It invokes wavedrom as a subprocess so it isn't dependant on the technology you're using to implement it.

The consequence of using nw.js for the WaveDrom CLI is that it can't run on a headless server. This restricts where asciidoctor-diagram users can use this integration. Running it on your typical CI server for instance is not possible. There's an issue open for this in the nw.js project (nwjs/nw.js#769).

I think the simplest solution for this problem is to also provide a CLI wrapper for wavedrom that uses phantom.js. phantom.js is explicitly designed to run in headless server environments.
I personally wouldn't try to remove all DOM dependencies from your code. That feels like a lot of work for very little added value. phantom.js provides an entire headless browser environment (including the DOM) so I don't think this will be necessary either.

The Mermaid diagram generator has a phantom.js based CLI wrapper that provides a good example of how this can be done (see https://github.com/knsv/mermaid/blob/master/bin/mermaid.js and https://github.com/knsv/mermaid/blob/master/lib/cli.js).

@mojavelinux
Copy link
Member

provide a CLI wrapper for wavedrom that uses phantom.js

100%

@drom
Copy link

drom commented Jan 20, 2016

@pepijnve @mojavelinux Done! PhantomJS version of WaveDrom CLI tool is here:
https://github.com/wavedrom/cli
Try it, let me know

@pepijnve
Copy link
Member

d0b09c7 makes asciidoctor-diagram look for wavedrom-cli first and falls back on wavedromeditor. Just ran the specs and seems to work as intended.

I ran into one small quirk: if the output file does end with '.png' then no output is generated. This doesn't happen when generating svg files. Not a big deal; I worked around it in the code.

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

No branches or pull requests

4 participants