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

HTML format: offline_version parameter #2532

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

Conversation

ilancoulon
Copy link

Adresses JuliaLang/julia#19354 and one of the items on #212.

The general idea is to try to add a keyword parameter to the HTML() struct. By putting format = Documenter.HTML(offline_version = true) in the parameters of makedocs, the HTML output should come bundled with all the CDNs.

Here is a list of what I've done:

  • added the keyword as a parameter of HTML
  • when a remote dependency is used in the HTMLWriter, I call a function of RD in between, called process_remote. This function, depending on the user-given parameter will download the dependency using curl and write it in the right file, replacing the CDN url with a relative URL.
  • for CSS font files, the URL inside are relative, so those files also need to be downloaded, this is done through the function _process_downloaded_css (with a regex)
  • for my internal use, having font files as an asset behind an authenticated portal was not working. I had to actually write them as a base64 encoded string inline the CSS file. This might not be useful for everyone but it makes docs displayable when the website needs authentication even for assets (because a font file is always fetched in an anonymous mode).
  • a limited test of the offline_version build. The rest of those functions is hard to test because this depends on curl.

I might have missed some dependencies or could improve some of the functions signature, would gladly take some feedback!

@mortenpi
Copy link
Member

Broadly, I think this looks pretty good, and it would be great to have this option. Thank you for taking a stab at this!

One thing that stands out to me is that we're modifying all the signatures for the different libraries in RD.jl. But it's really an all-or-nothing, not a per-dependency thing, right? I wonder if we could move the handling of all this into the shared JSDependencies.jl?

src/html/RD.jl Outdated
Comment on lines 165 to 174
function _process_downloaded_css(file_content, origin_url)
url_regex = r"url\(([^)]+)\)"
replace(file_content, url_regex => s -> begin
rel_url = match(url_regex, s).captures[1]
url = normpath(dirname(origin_url), rel_url)
font_type = font_ext_to_type[splitext(rel_url)[end]]
encoded_file = Base64.base64encode(_download_file_content(url))
return "url(data:font/$(font_type);charset=utf-8;base64,$(encoded_file))"
end)
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit non-trivial. It would be good to have a comment for it.

src/html/RD.jl Outdated
Comment on lines 137 to 138
@error "offline_version requires curl."
error()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@error "offline_version requires curl."
error()
error("offline_version requires curl.")

src/html/RD.jl Outdated
_process(dep::RemoteLibrary, build_path, origin_path) = RemoteLibrary(dep.name, _process(dep.url, build_path, origin_path); deps = dep.deps, exports = dep.exports)

function _download_file_content(url::AbstractString)
cmd = `curl $url -s --output -`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could also use Downloads.jl here. But curl is what we use elsewhere right now, and while we want to move away from it (#2254), its also completely fine to keep it as is.

@ilancoulon
Copy link
Author

Broadly, I think this looks pretty good, and it would be great to have this option. Thank you for taking a stab at this!

Thank you! I tried applying your comments. I basically added some comments, and used Downloads.jl instead of curl, it's much better!

One thing that stands out to me is that we're modifying all the signatures for the different libraries in RD.jl. But it's really an all-or-nothing, not a per-dependency thing, right? I wonder if we could move the handling of all this into the shared JSDependencies.jl?

I agree it's not nice to have to change these signatures and tried avoiding that. At some point I was indeed using JSDependencies but the thing is that all these functions in RD.jl actually convey a lot of information to JSDependencies when constructing the RemoteLibraries. And they need that build_path/output_path at this moment. I'd be happy to improve this part of the code but I don't see how yet, without modifying the whole RD.jl file, changing completely how it interacts with JSDependencies. Happy to hear a more specific idea on this if you have it.

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