[Feature request] Synchronous API

[christianvoigt]

Hi, I just discovered your fork. Your Changelog sounds very promising. What are your plans for this fork? Are you planning to offer a constantly maintained alternative to the original version or is this intended as an experiment or a project for personal use only?

The demise of viz.js was really a shame for a lot of projects as the only other real alternative for hierarchical layouts (dagre) was also more or less abandoned and is still lacking a lot of important features. You would definitely do a lot of people a great service if you were willing to bring viz.js to life once more.

I was the author of the last unanswered issue in this project: mdaines#165

If I understand the Changelog correctly, in your version it will be even more difficult to use viz.js synchronously. I think that would be a mistake as there are contexts where asynchronous calls simply are not possible and performance is not so important. I have no experience with worker threads in node.js, but do you think it would at least be possible to use https://www.npmjs.com/package/deasync with them?

Anyhow, thanks for your work, I hope this was not too intrusive, I am just really enthusiastic about the prospect of someone maintaining a new viz.js after all this time.

[aduh95]

Hi!

Are you planning to offer a constantly maintained alternative to the original version or is this intended as an experiment or a project for personal use only?

I plan to maintain it and have started using it for my own projects. I haven't released a stable version yet because my fork is using some experimental Node.js features (ESM, worker_threads), I am waiting for them to land on LTS line.

If I understand the Changelog correctly, in your version it will be even more difficult to use viz.js synchronously.

Currently, the whole API is async indeed. I am not opposed to provide a sync version alongside though, I haven't came up with an elegant solution so far. Maybe something like that?

import { renderStringSync } from '@aduh95/viz.js/sync'
renderStringSync("digraph{1 -> 2 }"); // <svg ...>

would at least be possible to use deasync with them?

I don't see why not, but I don't have a sufficient knowledge of deasync.

[aduh95]

@christianvoigt do you want to have a look to the linked PR to check if my implementation fits your use-case?

[christianvoigt]

That's really great news! Thanks for the PR, I will test it and get back to you here.

[christianvoigt]

Ok, I tried to build the project after installing emscripten, but I get some errors when running make deps. I tried the Dockerfile as well with the same results. First I get this from line 175 in the Makefile:

grep: build-full/graphviz-2.45.20200410.2133/graphviz_version.h: No such file or directory

There is only a graphviz_version.h.cmake file in my graphviz folder.

I removed the line from the make file and tried it without it. That produced an error in the next line:

/bin/sh: 1: ./configure: not found

In both cases some files in the build-full/graphviz folder are missing. Maybe something changed in the newest graphviz version?

I am trying to do this under Windows, but using WSL2, so I have used apt-get to install all dependencies listed in the Readme except "qt", because I ran into some trouble installing it under Linux. Might that cause these problems?

[aduh95]

Thanks for your feedback, I am facing similar issues trying to setup GitHub Actions. I am going to try to update Emscripten version and switch to a stable release of Graphviz to see if the setup gets easier.

Regarding the Dockerfile, since I'm not using it for development, I didn't take time to maintain and update it; I'm probably just gonna delete it for the time being.

[aduh95]

@christianvoigt You can use the GitHub Action script to help you set up your environment, otherwise you can use the build artefact of the attached PR to test locally.

[christianvoigt]

Thank you for your help! I can build the project now and have tested it for christianvoigt/argdown. The main issue currently is the use of subpath exports in the package.json file. Even though I have installed node v13.13.0 locally, Typescript is still not supporting this (see this issue). I simply changed the package.json to test the renderSync file. Apart from the subpath issue it works perfectly.

Even if the subpath feature lands in LTS I think I can not use it in Argdown in the next half year or so. Argdown provides a VSCode plugin to its users. VSCode is based on Electron and Electron has just updated to Node v12.13.0 in version 8. VSCode is not even using Electron 8 yet, so it will take some time until Node v13 (or 14) arrives in VScode.

A workaround would be to create a second package.json for older node versions as this might be a problem for other users as well. Otherwise, the only option I see right now is to fork your project and change the package.json to suit my needs (or even copy the source file directly into my project). What do you think? I think in the long run it will be great that this fork uses modern node features, but for now it makes it harder to switch.

[aduh95]

I have included a sync file containing a CJS export to the actual file to support older Node.js version/bundling tool:

viz.js/Makefile

Lines 112 to 113 in 886efaa

	sync:
	echo "module.exports=require('./dist/renderSync.cjs')" > $@

But I forgot to add it to the package.json files field, my bad. I've just fixed it in the PR, PTAL.

[christianvoigt]

I just realized that I could import renderSync.cjs with:

const vizRenderStringSync = require("../../node_modules/@aduh95/viz.js/dist/renderSync.cjs");

So maybe the package.json does not have to change. This will not work using "import" (in typescript), though.

How does the "sync" file in the root folder change that? At the moment I think I can not import it because it is missing a file extension (Typescript is not finding the module). If I change that to ".js" Typescript complains about a missing type declaration, so maybe it would work? I don't really get why including this file makes a difference from simply including "dist/renderSync.cjs" in the package.files section. Could you explain that?

By the way, a test in the CI action failed after your change.

[christianvoigt]

ah, sorry for the confusion. I think I get it now. So you can simply use the extension-less "sync" file as a fallback for the package export, that's great! I did not know that.

Once I realized this, I simply added

declare module "@aduh95/viz.js/sync";

to my "types/index.d.ts" and now I can simply use:

import vizRenderStringSync from "@aduh95/viz.js/sync";

This is great! Thank you for your effort!

[christianvoigt]

I just repeated my test using nvm in node v12.8.1 and node v10.20.1 and it works in both versions.

[aduh95]

@christianvoigt Thanks for your input, I have added a .d.ts file for @aduh95/viz.js/sync to improve the TypeScript dev experience, do you want to take another look? You should not need the declare module workaround hopefully.

By the way, a test in the CI action failed after your change.

Yep that's an issue on GH Actions actually, and the fix has already been merged so I won't bother fixing it on my side ;)

[christianvoigt]

I just checked and it now works without declare module. Great!

When do you plan to release your fork as an npm package? According to https://nodejs.org/en/about/releases/ node 14 should be released tomorrow (if not postponed because of the Corona virus). Node 13 should already be in "maintanance lts".

I just realized you already released the beta version to npm.js. It would be great if you could release the sync-api as well so that I can release a new Argdown version based on your fork.

From my point of view it would make sense to release viz.js officially even if worker-threads have not landed in lts, as the sync-api and the browser version do not depend on node worker threads and you have added fallbacks for the subpath exports. Maybe it would be helpful to mention the minimal required node version for the async usage in the Readme.

[christianvoigt]

I found another small Typescript issue: In line 1 in dist/renderSync.d.ts you import from ./types. I think this should be ./index:

import type { RenderOptions } from "./types";

[aduh95]

I'm planning on releasing a new beta version as soon as the PR is ready to be merged. I don't expect to make any other breaking changes to the sync API, so feel free to treat it as a Release Candidate.

I found another small Typescript issue: In line 1 in dist/renderSync.d.ts you import from ./types. I think this should be ./index

Yep I should a tests for that kind of behaviour, thanks for the report.