[kroki-fetch-diagram] generated-file-name parameter not used

[anb0s]

see #47 for my setup

My test presentation.adoc sets the imagesdir and use generated-file-name parameter to have all images with human readable names:

ifndef::imagesdir[:imagesdir: ./images]

== UML class diagram test with kroki
[plantuml, "diagram-classes", svg]
....
class BlockProcessor
class DiagramBlock
class DitaaBlock
class PlantUmlBlock

BlockProcessor <|-- DiagramBlock
DiagramBlock <|-- DitaaBlock
DiagramBlock <|-- PlantUmlBlock
....

It fetches the diagram and writes to images/210a0ebf2a7bb40669c4e3c895b2c9396f048327.svg instead of images/diagram-classes.svg like ascidoctor-diagram does.

It would be good to support the mandatory diagram parameters from asciidoctor-diagram.

[ggrossetie]

It fetches the diagram and writes to images/210a0ebf2a7bb40669c4e3c895b2c9396f048327.svg instead of images/diagram-classes.svg like ascidoctor-diagram does.

I need to check how Asciidoctor diagram resolve conflicts:

[plantuml, "diagram-classes", svg]
....
....

[plantuml, "diagram-classes", svg]
....
....

As far as I understand, the last diagram will win: https://github.com/asciidoctor/asciidoctor-diagram/blob/66876e9c11c401eec943ff6968cd0c5f6c443906/lib/asciidoctor-diagram/diagram_processor.rb#L181

[anb0s]

Yes it overwrites the file and it's OK, because i think conflict resolution would be complex to implement and user is responsible to use unique names or just not enter any generated-file-name to have it generated and unique...

[ggrossetie]

@anb0s If you want to give it a try, the line to update is:

https://github.com/Mogztter/asciidoctor-kroki/blob/b0575d075ea7088695f14318ca2969091e5b0267/src/fetch.js#L20

The "diagramName" (ie. file name) should be: ${target}.${format}.
Once it's done, we should probably remove the dependency on rusha since we won't need it anymore and update and/or add a test to make sure that it's working as expected.

[anb0s]

@anb0s If you want to give it a try, the line to update is:

https://github.com/Mogztter/asciidoctor-kroki/blob/b0575d075ea7088695f14318ca2969091e5b0267/src/fetch.js#L20

The "diagramName" (ie. file name) should be: ${target}.${format}.

Thanks, i will see how to create own version of asciidoc-kroki and try it.

Once it's done, we should probably remove the dependency on rusha since we won't need it anymore and update and/or add a test to make sure that it's working as expected.

I think we still need the auto-generated name, because target is optional:
The second, optional positional attribute assigns a file name (i.e. target) to the generated diagram. If a target is not specified an auto-generated name will be used.

So if target is empty, or how ever it's passed as if not valid/given, then it should still use the code with rusha dependency.

[ggrossetie]

Thanks, i will see how to create own version of asciidoc-kroki and try it.

Awesome, let me know if you need help to setup the project on your machine.

I think we still need the auto-generated name, because target is optional:
The second, optional positional attribute assigns a file name (i.e. target) to the generated diagram. If a target is not specified an auto-generated name will be used.
So if target is empty, or how ever it's passed as if not valid/given, then it should still use the code with rusha dependency.

Oh you're right.

I believe that Asciidoctor Diagram is using the following code when the file name (ie. target) is absent: https://github.com/asciidoctor/asciidoctor-diagram/blob/66876e9c11c401eec943ff6968cd0c5f6c443906/lib/asciidoctor-diagram/diagram_source.rb#L156
So basically what we are doing minus the diag- prefix.

[anb0s]

@Mogztter i'm new to JS and tried to setup dev environment with npm @ windows (the only system at work :( ), but no luck:

12 info lifecycle [email protected]~test: Failed to exec test script
13 verbose stack Error: [email protected] test: `npm run test:node && npm run test:browser`
13 verbose stack Exit status 1
13 verbose stack     at EventEmitter.<anonymous> (d:\Progs\node-v12.16.2-win-x64\node_modules\npm\node_modules\npm-lifecycle\index.js:332:16)
13 verbose stack     at EventEmitter.emit (events.js:310:20)
13 verbose stack     at ChildProcess.<anonymous> (d:\Progs\node-v12.16.2-win-x64\node_modules\npm\node_modules\npm-lifecycle\lib\spawn.js:55:14)
13 verbose stack     at ChildProcess.emit (events.js:310:20)
13 verbose stack     at maybeClose (internal/child_process.js:1021:16)
13 verbose stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:286:5)
14 verbose pkgid [email protected]
15 verbose cwd c:\Users\kenobi\dev\asciidoctor-kroki
16 verbose Windows_NT 10.0.18363
17 verbose argv "d:\\Progs\\node-v12.16.2-win-x64\\node.exe" "d:\\Progs\\node-v12.16.2-win-x64\\node_modules\\npm\\bin\\npm-cli.js" "run" "test"
18 verbose node v12.16.2
19 verbose npm  v6.14.4
20 error code ELIFECYCLE
21 error errno 1
22 error [email protected] test: `npm run test:node && npm run test:browser`
22 error Exit status 1
23 error Failed at the [email protected] test script.
23 error This is probably not a problem with npm. There is likely additional logging output above.
24 verbose exit [ 1, true ]

I think windows is not good env for development and i will use my manjaro linux system at home.

But if it should work at windows please give me a hint :)

[ggrossetie]

@anb0s the stacktrace is very vague... my guess is that Puppeteer cannot launch an headless Chrome. Could you please try to run npm run test:node?

[anb0s]

I've enabled --verbose but still no more information about the reason:

0 info it worked if it ends with ok
1 verbose cli [
1 verbose cli   'd:\\Progs\\node-v12.16.2-win-x64\\node.exe',
1 verbose cli   'd:\\Progs\\node-v12.16.2-win-x64\\node_modules\\npm\\bin\\npm-cli.js',
1 verbose cli   '--verbose',
1 verbose cli   'run',
1 verbose cli   'test:node'
1 verbose cli ]
2 info using [email protected]
3 info using [email protected]
4 verbose run-script [ 'pretest:node', 'test:node', 'posttest:node' ]
5 info lifecycle [email protected]~pretest:node: [email protected]
6 info lifecycle [email protected]~test:node: [email protected]
7 verbose lifecycle [email protected]~test:node: unsafe-perm in lifecycle true
10 silly lifecycle [email protected]~test:node: Args: [ '/d /s /c', 'mocha test/*.js' ]
11 silly lifecycle [email protected]~test:node: Returned: code: 1  signal: null
12 info lifecycle [email protected]~test:node: Failed to exec test:node script
13 verbose stack Error: [email protected] test:node: `mocha test/*.js`
13 verbose stack Exit status 1
13 verbose stack     at EventEmitter.<anonymous> (d:\Progs\node-v12.16.2-win-x64\node_modules\npm\node_modules\npm-lifecycle\index.js:332:16)
13 verbose stack     at EventEmitter.emit (events.js:310:20)
13 verbose stack     at ChildProcess.<anonymous> (d:\Progs\node-v12.16.2-win-x64\node_modules\npm\node_modules\npm-lifecycle\lib\spawn.js:55:14)
13 verbose stack     at ChildProcess.emit (events.js:310:20)
13 verbose stack     at maybeClose (internal/child_process.js:1021:16)
13 verbose stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:286:5)
14 verbose pkgid [email protected]
16 verbose Windows_NT 10.0.18363
17 verbose argv "d:\\Progs\\node-v12.16.2-win-x64\\node.exe" "d:\\Progs\\node-v12.16.2-win-x64\\node_modules\\npm\\bin\\npm-cli.js" "--verbose" "run" "test:node"
18 verbose node v12.16.2
19 verbose npm  v6.14.4
20 error code ELIFECYCLE
21 error errno 1
22 error [email protected] test:node: `mocha test/*.js`
22 error Exit status 1
23 error Failed at the [email protected] test:node script.
23 error This is probably not a problem with npm. There is likely additional logging output above.
24 verbose exit [ 1, true ]

[ggrossetie]

@anb0s Did you install the dependencies using npm i? Do you have a node_modules directory at the root of the project? Any errors or warnings during npm i?

If you did, could you try to execute npm bin? It will output a directory, then look into this directory and check if you have a mocha binary.

[anb0s]

@Mogztter yes i've tried the steps described. I see no warnings.

After more research i've executed npm cache clean --force and now with npm run test:node it executes more and mocha.cmd etc. are in the node_modules\.bin folder.

6 Test failed:

> mocha test/*.js

  preprocessVegaLite
    √ should throw an error for invalid JSON
    √ should return original diagramText for valid JSON but without "data.url"
    √ should throw an error for unexisting local file referenced with relative path
    √ should throw an error for unexisting local file referenced with "file://"
Skipping preprocessing of Vega-Lite view specification, because reading the referenced remote file 'https://raw.githubusercontent.com/Mogztter/asciidoctor-kroki/master/unexisting.csv' caused an error:
Error: No such file: https://raw.githubusercontent.com/Mogztter/asciidoctor-kroki/master/unexisting.csv
    √ should warn and return original diagramText for unexisting remote file referenced with "data.url", because it can perhaps be found by kroki server (369ms)
    1) should return diagramText with inlined local file referenced with "data.url"
    √ should return diagramText with inlined remote file referenced with "data.url" (193ms)

  Registration
    √ should register the extension

  Conversion
    When extension is registered
      √ should convert a diagram to an image (51ms)
      2) should convert a diagram with an absolute path to an image
      3) should convert a diagram with a relative path to an image
      √ should download and save an image to a local folder (272ms)
      √ should apply substitutions in diagram block
      4) should apply attributes substitution in target
      √ should not download twice the same image (288ms)
      √ should create a literal block when format is txt (308ms)
      √ should read diagram text
      √ should embed an SVG image with built-in allow-uri-read and data-uri (available in Asciidoctor.js 2+) (935ms)
      √ should inline an SVG image with built-in allow-uri-read (available in Asciidoctor.js 2+) (325ms)
      √ should inline an SVG image with kroki-fetch-diagram (838ms)
      √ should include an interactive SVG image with built-in allow-uri-read and data-uri (available in Asciidoctor.js 2+) (982ms)
      5) should include an interactive SVG image with kroki-fetch-diagram
      √ should convert a PacketDiag diagram to an image
      √ should convert a RackDiag diagram to an image
      √ should convert a Vega diagram to an image
      √ should convert a Vega-Lite diagram to an image
      6) should inline a referenced data file for a Vega-Lite diagram and convert to an image
      √ should convert a WaveDrom diagram to an image
      √ should convert a BPMN diagram to an image
      √ should convert a Bytefield diagram to an image

  24 passing (5s)
  6 failing

[ggrossetie]

That's better!
The failures are probably related to the path separator being \ on Windows and not /. I will add Windows on the CI to fix these issues.

Edit: confirmed, I have the same result: https://github.com/Mogztter/asciidoctor-kroki/pull/66/checks?check_run_id=660834778. It's related to new lines being \n\r on Windows 😵

[anb0s]

Yes this test fails, because of line separator expected \n and is on windows \r\n

 1) preprocessVegaLite
       should return diagramText with inlined local file referenced with "data.url":

      AssertionError: expected '{"data":{"values":"a,b,c\\r\\n2020-01-05,0.3,C1\\r\\n2020-01-15,0.7,C1\\r\\n2020-01-05,0.5,C2\\r\\n2020-01-15,0.8,C2\\r\\n","format":{"type":"csv"}}}' to equal '{"data":{"values":"a,b,c\\n2020-01-05,0.3,C1\\n2020-01-15,0.7,C1\\n2020-01-05,0.5,C2\\n2020-01-15,0.8,C2\\n","format":{"type":"csv"}}}'
      + expected - actual

      -{"data":{"values":"a,b,c\r\n2020-01-05,0.3,C1\r\n2020-01-15,0.7,C1\r\n2020-01-05,0.5,C2\r\n2020-01-15,0.8,C2\r\n","format":{"type":"csv"}}}
      +{"data":{"values":"a,b,c\n2020-01-05,0.3,C1\n2020-01-15,0.7,C1\n2020-01-05,0.5,C2\n2020-01-15,0.8,C2\n","format":{"type":"csv"}}}

[anb0s]

created new issue #68 for failed tests @ windows, continue there :)

[mojavelinux]

The linked PR is closed, however this issue is still open. In my trials (using the RubyGem), it appears that the custom / human-readable name is still ignored (https://github.com/Mogztter/asciidoctor-kroki/blob/master/ruby/lib/asciidoctor/extensions/asciidoctor_kroki/extension.rb#L291). Is that accurate? If so, is the plan still to support the custom name, or is there a reason for always using the auto-generated name?

I'd be glad to help get this resolved.

[ggrossetie]

It was reverted because we are using the filename as the cache key: #90
So we need to find another way to reliably determine if the content has changed or not. Another issue is that it might overwrite existing files if two (or more) diagrams have the same custom name. In this case, I think we should probably issue a warning and let the user/writer fix this issue? Another idea is to append an incremental id but it complicated the logic a bit.

[mojavelinux]

Thank you for providing that context. Now that I understand the challenges, I'd like to propose an alternate approach. As a user, I don't necessarily want to control the entire filename. Rather, my wish is to be able to control the filename so I'm able to recognize which diagram file goes with which diagram. To achieve that goal, I propose using the diagram name specified on the block as the filename prefix in place of diag (e.g., diagram-name-cf699e6a13a1976b55764d80b6f9d508.png). That makes the filename recognizable while still including the hash to serve as the cache key and to prevent filename conflicts. wdyt?

[ggrossetie]

To achieve that goal, I propose using the diagram name specified on the block as the filename prefix in place of diag (e.g., diagram-name-cf699e6a13a1976b55764d80b6f9d508.png).

That's a good idea!
I wonder if it exists a better (shorter) hashing algorithm to make the filename more human-friendly? I did a quick search but I couldn't find a good alternative.

That makes the filename recognizable while still including the hash to serve as the cache key and to prevent filename conflicts. wdyt?

I agree, let's do that 👍🏻

[vy]

Currently, [ditaa,foo] generates foo-cf699e6a13a1976b55764d80b6f9d508.png. It is pretty common that search engines and users link to these image files. Whenever I make a change in the diagram, the checksum will be updated, and the old filename will start giving 404s. Is it possible to exclude the -<checksum> part completely to avoid this? Or is there any other way to approach to this problem?

[ggrossetie]

@vy could you please open a new issue since this one is closed, thanks!