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

Replace html-css-js-minify with minify_jll, eliminate Python3 dependency #1038

Closed
wants to merge 11 commits into from

Conversation

M-PERSIC
Copy link

@M-PERSIC M-PERSIC commented Jul 30, 2023

I recently ported over Taco de Wolff's Minify to Julia as a JLL package and felt it would be an excellent addition to Franklin.

Advantages

  • Dependency packaged directly as a JLL
  • Additional support for json, svg, and xml minification
  • Eliminate Python3 as an external dependency (I commented out the internal functions if they are ever needed again for some future dependency, though I can remove them outright if desired)

Disadvantages

  • Would need to bump Franklin to julia versions 1.6+. I could re-add the python3 minifer and ensure it is only used for julia v1.5 and below.
  • Minor performance regression (2x based on some side-by-side comparisons with the included templates, though it might scale better with larger sites)

Considerations

  • I did not touch any GitHub actions for this pr as I'm unsure if we would still prefer installing python3 for whichever reason
  • If there are other packages that we would like to integrate into Franklin I'd be more than happy to work on uploading them as JLLs to avoid more external dependencies in the future

@M-PERSIC M-PERSIC changed the title Replace html-css-js minify with minify_jll, eliminate Python3 dependency Replace html-css-js-minify with minify_jll, eliminate Python3 dependency Jul 30, 2023
@M-PERSIC M-PERSIC marked this pull request as draft July 31, 2023 00:13
@tlienart
Copy link
Owner

Hello @M-PERSIC , thanks a lot for this, it's awesome and the PR is nice and clean. I will not merge this into Franklin however as the current status is kind of like an LTS release. The next version of Franklin is in preparation at Xranklin.jl and about 95% ready (mostly docs stuff at this point).

The next version drops these external dependencies which caused issues and ultimately were not that useful.

minify_jll sounds great but it's the kind of stuff which I'd like users to opt-in via their utils file rather than have it baked in the code.

Comments / next steps:

  1. there isn't yet an explicit spot in the next version of Franklin to specify post-build logic; this is where a user who would want to benefit from minify_jll would add a few lines to indicate they want to do so. If you have thoughts on where this could be placed (e.g. in utils.jl or in a separate file, or as an argument to build(...)) and, more generally, if you're interested in porting this as a plugin to this next version, please kindly open an issue at Xranklin.jl for further discussion
  2. in your code, you call success(`$(minify_jll.minify()) -o $(filepath) $(filepath)`), I'm not very familiar with jll files but can't these be called directly within Julia? does it have to be shelled out? not an issue, just curiosity here.

Thanks again for your proposal!

@M-PERSIC
Copy link
Author

@tlienart Thank you for your feedback!

  • I think minification should be a fundamental component of Xranklin's build process as its advantages are too numerous. Some quick brainstorming, minify_jll could be included as a package extension and a minify option is added to the file watching process via full_pass(). I'll open an issue in Xranklin hopefully soon once I have some more concrete ideas.
  • It depends on what is included in the JLL package if it must be shelled out or not. If a C/Fortran-ABI compatible shared library is included, it can be invoked via Libdl or ccall. If an executable is included, then it must be run as an external program, though because it is fully packaged into Julia with the appropriate wrappers it is treated as a first-class Julia package. A good summary of JLL packages is in Pkg + BinaryBuilder -- The Next Generation

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