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

add possibility to register directories #417

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

Conversation

hhaensel
Copy link

This is the corrseponding PR to issue #416

Currently, WebIO does not allow for serving of directories.

I propose to slightly modify asset.jl to allow for the serving of directories. This is a prerequisite to adapt PlotlyJS to include mathjax in its output.

To me it looks as if the design is already made for this purpose, but is not fully completed yet. I think one return was forgotten and I chose to use SubString to remove the initial path but include the separator.

@hhaensel
Copy link
Author

hhaensel commented Jun 1, 2020

I've added another commit, which does not cut the query string at a potential '?'.
It's even a bit shorter ;-)
But I wonder whether baseurl[] * should not be omitted here as it already occurs in AssetRegistry.getkey. Instead it should probably be added in line 185 before "/assetserver/". Please correct me, if I'm wrong.
One last question. Currently AssetRegistry.isregistered() computes the key everytime. Wouldn't it be better to check for the value instead, i.e.

isregistered(path) = abspath(path) in values(registry)

instead of

isregistered(path) = haskey(registry, getkey(path))

Getkey could be made faster along the same lines by first checking existing entries.

generatekey(path) =  baseurl[] * "/assetserver/" * bytes2hex(sha1(abspath(path))) * "-" * basename(path)

function getkey(path)
    key = findfirst(x -> x == abspath(path), registry)
    key === nothing ? generatekey(path) : key
end

I could file a PR for AssetRegistry as well, if this is welcome.
The main time killer, however, is abspath(), at least on my windows machine ...

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.

1 participant