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

Support PlantUML; Ordering in [[index]] #37

Closed
wants to merge 5 commits into from

Conversation

atifsyedali
Copy link
Contributor

@atifsyedali atifsyedali commented Jan 7, 2019

Resolves #35, #10, #36, and #38.

@atifsyedali atifsyedali changed the title Support PlantUML Support PlantUML; Ordering in [[index]] Jan 7, 2019
@sinedied
Copy link
Owner

Thanks for the PR!
I'll take a look at it as soon as I can 😃

The CI is not passing, I think the lockfile is not up to date regarding the package.json modification, can you try to update it using a recent NPM version?

I also have a question, as I quickly took a look at the https://www.npmjs.com/package/node-plantuml-vizjs package: from the readme, it seems it still needs graphviz installed on the local machine, which would be problematic. I also seems to be based on Java, even though the Java requirement isn't explicit.

I'll try to test out these in an isolated environment, as hads needs to work on mac/win/linux without any prerequisites. A possible workaround would be to only activate plantuml support using a flag, stating all requirements for it to work, even though I would prefer to avoid that if possible.

@atifsyedali
Copy link
Contributor Author

The CI is not passing, I think the lockfile is not up to date regarding the package.json modification, can you try to update it using a recent NPM version?

Sorry about that. Done.

from the readme, it seems it still needs graphviz installed on the local machine, which would be problematic.

The package node-plantuml-vizjs is actually published from my fork of node-plantuml https://github.com/atifsyedali/node-plantuml where I have removed the hard graphviz dependency. I didn't update the repo path on the package.json which still points to the original, mainly because I expect the PR markushedvall/node-plantuml#26 to be merged into the master repo. Having said that, you can wait for markushedvall/node-plantuml#26 PR to be merged in first before merging this PR.

I also seems to be based on Java, even though the Java requirement isn't explicit.

Yes, but only required for running generating plantuml diagrams, not for installation. We can use something like https://www.npmjs.com/package/node-jre but I feel it would be an overkill.

@sinedied
Copy link
Owner

Hello, I'm very sorry for huuuuuge delay I started to write a response then got interrupted with something and... plainly forgot, my bad 😞

I tried you branch in multiple situations (with Java installed, without, with graphviz, etc) and it's working as expected, except that it's painfully slow on my machine (macbook 12"): it takes ~18-20s to render 1 image from the test markdown whatever the setup, and completely block rendering until done, which is not really ideal.

In addition, it adds ~16MB to the whole package install which is +15% compared to the current install (that is already huge). Thing is, I planned to initially slim down the whole under 15MB by adding a build step for the frontend libs, so if I merge this I won't be able to achieve this goal.

But ultimately, I'm sure your work would be useful to others as it was requested before and I have been working for some time on a solution that would make everyone happy, by having a simple plugin solution to add extra rendering & markdown extension as separate packages, allowing to avoid the bloat of a single all-in-one package.
Thing is, it's done on my spare time so I cannot give an ETA for plugin support, so the best propal I have for you currently is for you to release your actual fork as a new package, for example hads-plantuml, and I'll add a mention and link to it in the readme for those interested.

And once the plugin system is ready for prime time, I'll make the PR on your fork to turn into an actual plugin, if that's ok for you?

Let me know how does that sound to you, and thanks again for your work.

@atifsyedali
Copy link
Contributor Author

@sinedied I agree with all your points about the bundle size and the slow rendering. Feel free to make a mention to hads-plantuml -- it is already on npm as I needed it for a client. Thanks.

@damiendube
Copy link
Contributor

@atifsyedali Mind splitting your PR? I would like a fix for #36 and #38.
I can also do the split a push a PR with just this change in

@atifsyedali
Copy link
Contributor Author

@damiendube Sure, but I probably won't get to it soon. If you require it urgently you can use my fork https://github.com/atifsyedali/hads (published here https://www.npmjs.com/package/hads-plantuml).

@sinedied sinedied closed this Jan 11, 2021
@sinedied sinedied deleted the branch sinedied:master January 11, 2021 12:55
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.

PlantUML Support
3 participants