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

bugfix/fix-kroki-fetch-diagram-attribute-in-js-version #453

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

Conversation

AshkanV
Copy link

@AshkanV AshkanV commented Jun 12, 2024

Fixes the kroki-fetch-diagram attribute in NodeJS Version and also use the disk cache file if available

@ggrossetie
Copy link
Member

Sorry I didn't know that unxhr is the better option, as there was some debates which says unxhr cant be run in VSCode. So I though there are two issues, one with the library and one with implementation of the attribute.

I believe that node-xmlhttprequest will have the same issue since it's basically the same codebase and use the same mechanism to execute synchronous HTTP requests.

So I deleted my branch and will commit a clean one in a new branch and make another pull request.

👍🏻

By the way I am C++ developer and didn't find any doc in this repository on how to create a test and I myself is not familiar with the standard ways and workflows of a Node project. Could you please show me a clue on how to write a test for this project.

No worries.
In that case, could you describe a specific case (AsciiDoc file + environment) that currently doesn't work?

@AshkanV
Copy link
Author

AshkanV commented Jun 12, 2024

Thank you
This should be a fix for:
asciidoctor/asciidoctor-vscode#683
and probably:
#397

the sample document would be:

= Title
:safe: unsafe
:data-uri: 
:allow-uri-read:
:kroki-fetch-diagram:

== Subtitle

[plantuml]
----
@startuml
Bob -> Alice : Request
Alice -> Bob : Response
@enduml
----

I am using the latest VSCode and AsciiDoc extension. The only setting applied is enabling kroki.

if I replace :kroki-fetch-diagram: with :kroki-fetch-diagram!: the diagram is showing

@ggrossetie
Copy link
Member

I don't think it will fix this issue.
The reason kroki-fetch-diagram option does not work in VS code is because unxhr (and node-xmlhttprequest) are executing a Node script synchronously using:

spawn(process.argv[0], ["-e", execString])

https://github.com/driverdan/node-XMLHttpRequest/blob/97966e4ca1c9f2cc5574d8775cbdacebfec75455/lib/XMLHttpRequest.js#L507

require('child_process').execSync(`"${process.execPath}" ...`)

https://github.com/ggrossetie/unxhr/blob/4fbc39978b99180c24cd31ac802c2b4ae5dca3c6/lib/XMLHttpRequest.js#L520C60-L524

It works relatively well in a Node environment, since process.execPath (or process.argv[0]) will resolve to the path to the node binary.
Unfortunately, when running in VS code, Node is somehow embedded and cannot be used to run scripts. So process.execPath will resolve to the binary code which cannot execute Node scripts.

See ggrossetie/unxhr#98

I haven't try Enrik Berkhan solution but I believe it's the only way to fix this issue.

@AshkanV
Copy link
Author

AshkanV commented Jun 13, 2024

Thank you for your detailed description.

But after the changes in this pull request I was able to retrieve diagram files and have them embedded in the final HTML output and the export is single standalone HTML file again. Currently I am using these modifications on my local VSCode AsciiDoc plugin.

Before these changes, I needed to export using the Ruby version of AsciiDoctor to have an standalone HTML file.

@ggrossetie
Copy link
Member

But after the changes in this pull request I was able to retrieve diagram files and have them embedded in the final HTML output and the export is single standalone HTML file again. Currently I am using these modifications on my local VSCode AsciiDoc plugin.

Interesting... I will give it a try 👍🏻

@ggrossetie
Copy link
Member

I can confirm that unxhr seems to be working on the latest version of VS code. I can retrieve the image from Kroki and the file is also written on the disk:

image

The issue is that the file is written at the root of the repository but we try to load the image from the "output directory".
If I do something like (don't do that!):

image

Then it's working!

Anyway, what we should do is embed images as data URI if data-uri is set.

As a next step, we should try to find a solution to make it work in preview environment (such as VS code) when data-uri is not set.

@ggrossetie
Copy link
Member

Since we cannot use data-uri in VS code, I went for kroki-data-uri:

image

@AshkanV
Copy link
Author

AshkanV commented Jun 13, 2024

Thank you

As a matter of fact I always set the imagesoutdir to the temp folder of the OS and also set an attribute in the setting of the VSCode plugin for preview and check it using AsciiDoc syntax:

ifdef::vscode-preview[]
:kroki-fetch-diagram!:
endif::[]
ifndef::vscode-preview[]
:kroki-fetch-diagram:
endif::[]

So I get best performance in preview.

@AshkanV
Copy link
Author

AshkanV commented Jun 13, 2024

I think if I don't set the imagesoutdir the Ruby version also saves the file in the root directory of the document.

Since we cannot use data-uri in VS code, I went for kroki-data-uri:

Oh, I didn't know that data-uri is not working in VSCode. Because I did use it before to embed external images inside final HTML export and if I didn't use it, the result HTML just keep a URL to its location

@ggrossetie
Copy link
Member

Oh, I didn't know that data-uri is not working in VSCode. Because I did use it before to embed external images inside final HTML export and if I didn't use it, the result HTML just keep a URL to its location

Reading local files from VS code is not supported by Asciidoctor.js since we need to use a FileSystemProvider https://code.visualstudio.com/api/references/vscode-api#FileSystemProvider

@ggrossetie
Copy link
Member

I have merged your changes in #454 thanks again for the fix!

@AshkanV
Copy link
Author

AshkanV commented Jun 14, 2024

Thank you so much.

I read your final merged code to learn more about the internal functionality of the system and also how to write the required tests.

I have only two questions.

  1. Could you please describe a little more why couldn't we just use data-uri and needed a new attribute for it (kroki-data-uri)?
  2. Is the new code uses any file caching?

Because I saw that no file is ever created and so every time all the diagrams should be fetched again, which really takes quite some times on big documents that normally contains no changes in most of diagrams.

I tested the behavior with the Ruby version and it also creates fetched images in the imagesoutdir, so maybe it is more consistent if JS version also behaves similarly and provides the file caching mechanisms.

I also test it with my original pull request in VSCode environment:

  1. First exported an HTML from the test diagram (which creates a file in the imagesoutdir).
  2. Then I stopped the Kroki container and export another HTML with no server, and the file is read from the disk and embedded in the final HML.
  3. Also I delete/remove the cached image on the disk and retry to export. The resulting HTML just contains a text of the PlantUML diagram and no image and no link to the server is included (because of kroki-fetch-diagram attribute).

Thanks

@AshkanV
Copy link
Author

AshkanV commented Jun 14, 2024

If it is of any help, I also updated my code to remove extra parameter in save function and also return the full file path of the file in case of no data-uri attribute, And also included the test from the merged commit (with one change: kroki-data-uri -> data-uri)

@ggrossetie
Copy link
Member

Could you please describe a little more why couldn't we just use data-uri and needed a new attribute for it (kroki-data-uri)?

We support both but in VS code data-uri is forcibly disabled since reading local files from VS code is not supported by Asciidoctor.js and will throw an error.
Example:

= Title
:data-uri:

image:path/to/foo.png[]

The above document used to throw an error in VS code so we've decided to forcibly disable data-uri.

Is the new code uses any file caching?

No, we are reading the content and converting it to base64 in memory.

I tested the behavior with the Ruby version and it also creates fetched images in the imagesoutdir

When using data-uri? That's a bit unexpected. In my opinion if you want to embed images in your HTML document, we should not write them in the imagesoutdir. We could however write them in a "cache directory".
This is somehow related to #90 and something we could implement independently of data-uri (or kroki-data-uri).

@AshkanV
Copy link
Author

AshkanV commented Jun 14, 2024

Yes its is correct that data-uri is not supported directly in VSCode and the developer tool shows that only the image file address is there, but when we export an HTML with this attribute enalbed, it puts a base64 encoding of the image in the resulting file.

When using data-uri? That's a bit unexpected. In my opinion if you want to embed images in your HTML document, we should not write them in the imagesoutdir. We could however write them in a "cache directory".

I also think that is correct, but what about when data-uri is combined with kroki-fetch-diagram? Shouldn't it first fetch (and save the diagram to the disk) and then encode it in the resulting document?

Thank you

@AshkanV
Copy link
Author

AshkanV commented Jun 14, 2024

When using data-uri?

Yes, in my exprience:

This is the sample file (test.ad):

= Title
:data-uri:
:diagram-server-type: kroki_io
:diagram-server-url: https://kroki.io

[plantuml]
----
@startuml
Alice -> Bob: Authentication Request
Alice <-- Bob: Another authentication Response
@enduml
----

and the command to run is:

asciidoctor -r asciidoctor-diagram test.ad

This creates a picture file: (diag-plantuml-md5-98ade79bc4f3c15dbb15304e0097be58.png) and a HTML file with the image data base64 encoded in it.

@AshkanV
Copy link
Author

AshkanV commented Jun 15, 2024

I also tested the asciidoctor-kroki directly (instead of asciidoctor-diagram) and the result is:

  1. If I only use data-uri it only puts a link of the diagram to the server in the resulting HTML.
  2. If I use both data-uri and kroki-fetch-diagram it first downloads the file to the imagesoutdir (or beside the document, if not set) and then base64 encode it in the resulting HTML.

This is the sample file (test.ad):

= Title
:data-uri:
:kroki-fetch-diagram:

[plantuml]
----
@startuml
Alice -> Bob: Authentication Request
Alice <-- Bob: Another authentication Response
@enduml
----

and the command to run:

asciidoctor -r asciidoctor-kroki test.ad

@ggrossetie
Copy link
Member

ggrossetie commented Jun 15, 2024

It makes more sense to write files on disk when kroki-fetch-diagram is defined. So it seems that Asciidoctor Diagram automatically fetches and writes files on disk?

@AshkanV
Copy link
Author

AshkanV commented Jun 15, 2024

It makes more sense to write files on disk when kroki-fetch-diagram is defined. So it seems that Asciidoctor Diagram automatically fetches and writes files on disk?

Yes it looks like that "Asciidoctor Diagram" does that with only data-uri enabled and "Asciidoctor Kroki" (Ruby version) does that when both data-uri and kroki-fetch-diagram is set.

@ggrossetie
Copy link
Member

Sorry I lost track of this pull request. Can we close it or do we still need to do something?

@AshkanV
Copy link
Author

AshkanV commented Sep 16, 2024

Hello, Sorry I thought you might be busy, so I didn't bother anymore
I created a local branch for myself, which embed the diagram images if both kroki-fetch-diagram and data-uri attributes are set it fetches the diagram and save it on the disk which is used as a cache and also base64-encode it in the final output

But I see that after the changes made here, it has conflicts with current codes

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.

2 participants