sinedied / hads

:books: Markdown superpowered documentation for Node.js
MIT License
167 stars 28 forks source link

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

Closed atifsyedali closed 3 years ago

atifsyedali commented 5 years ago

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

sinedied commented 5 years ago

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 commented 5 years ago

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 https://github.com/markushedvall/node-plantuml/pull/26 to be merged into the master repo. Having said that, you can wait for https://github.com/markushedvall/node-plantuml/pull/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 commented 5 years ago

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 commented 5 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@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).