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

Implement a cache to fetch an image from the server only when the content has changed #90

Open
ggrossetie opened this issue Jun 4, 2020 · 7 comments · May be fixed by #399
Open

Implement a cache to fetch an image from the server only when the content has changed #90

ggrossetie opened this issue Jun 4, 2020 · 7 comments · May be fixed by #399

Comments

@ggrossetie
Copy link
Member

ggrossetie commented Jun 4, 2020

The filename of the image can be defined by the user. In this case, we cannot tell if we need to fetch the image again because the content has changed or if we don't because the content is the same.

Previously, we were using the md5sum of the diagram content as filename.
We might need to write a metadata file next to the images to compare the md5sum.

@ggrossetie
Copy link
Member Author

We should leverage XDG https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html and create a single JSON file that contains a map of filename with the associated md5sum of the source diagram.

//cc @danyill

@ggrossetie
Copy link
Member Author

Maybe we should revert 00a8a69 and release a new version. There's already a lot of changes!
Do you mind @anb0s?

@danyill
Copy link
Contributor

danyill commented Jun 11, 2020

How about post-processing svg data to add the metadata in there rather than having a manifest?

The same thing could be done with png (see the spec). That would be at some level "nicer" than publishing a manifest.

Do we support other asciidoctor-diagram formats? (pdf and txt)?

It would probably add somewhat to processing time but might be a "cleaner" although more intricate solution (and perhaps harder to support for the browser).

Thoughts?

@ggrossetie
Copy link
Member Author

Do we support other asciidoctor-diagram formats? (pdf and txt)?

Yes we support txt, pdf and jpeg but I think we can safely ignore pdf in this context.

It would probably add somewhat to processing time but might be a "cleaner" although more intricate solution (and perhaps harder to support for the browser).

Indeed, it means that we need libraries to understand png, svg and jpeg formats to add the additional metadata.
In a browser, we do not fetch the images so that won't be an issue until we do.

The same thing could be done with png (see the spec). That would be at some level "nicer" than publishing a manifest.

It might be one of those cases where "le mieux est l'ennemi du bien." (not sure how to translate that, probably best/perfect is the enemy of the good).
In other words, it sounds fancy and nice but I think we should keep it simple. I'm reluctant to alter the content of the images but it might be something we could implement in the Kroki server...?

@danyill
Copy link
Contributor

danyill commented Jun 11, 2020

Thanks for your thoughts @Mogztter I just wanted to add this idea into the mix. Yes we have the same expression "the perfect is the enemy of the good".

@ggrossetie
Copy link
Member Author

Maybe we should revert 00a8a69 and release a new version.

I will move forward and do that. It will give us some time to think it through.

Thanks for your thoughts @Mogztter I just wanted to add this idea into the mix. Yes we have the same expression "the perfect is the enemy of the good".

Thanks, it's much appreciated and beneficial to explore all the possible solutions 👍

@ggrossetie ggrossetie changed the title Find a way to not fetch the same image again when using a user-defined filename Implement a cache to fetch an image from the server only when the content has changed Jun 15, 2020
@ggrossetie
Copy link
Member Author

We could implement different strategies (configurable via attributes):

For reference, currently the cache is host dependent, ignores server cache and local cache cannot be disabled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants