Always vendor all external JS/CSS

[bollwyvl]

Alternative to #166:

• remove cdn configuration option, assume we'll be vendoring
• remove links to CDN altogether (no more version matching)
• connect to earlier lifecycle event than when mathjax gets injected, so can remove all the version detection cruft
  • no longer handles mathjax
• webpack generates a webpack-macros.html which is imported by layout.html to generate links to cache-busting filenames
  • this handles (part of) Configure Webpack to load fonts & regular css files #173
  • first party code includes the hash index.<hash>.css, index.<hash>.js
    • index.*.js is also set to preload in the head: this allows browsers to fetch it while the initial page draw is occurring, ratcheting time-to-first interaction up a bit
  • vendored packages have the package version extracted from package.json and appened after the name

[bollwyvl]

Aside from my fumbling, the deployed assets are actually getting put in place... however, the woff assets are 403ing...

Since this adds the node build, it may be worth doing further work on caching:

• cache miniconda download?
  • and/or use miniforge?
• cache the yarn packages
  • i'm actually a fan of its offline mirror feature, though it would be another discussion whether to just take advantage of it locally (and cach in ci), or whether to actually check in all the tarballs, and start using --offline and --frozen-lockfile
  • at present, that would cost:

du -h .yarn-packages/
42M	.yarn-packages/

• ... and of course, node-sass still gets compiled every time, but it's very hard to get left-padded when you always have your whole build chain and all dependencies around

[bollwyvl]

Also happy to upstream #167 into this, if that is the preferred approach, but getting the fonts and mathjax is pretty important to me as well.

[hoetmaaiers]

HI @bollwyvl , I prefer this approach in favour of #166, great you provided both options!

It would be nice if you could upstream #167, I did take a different approach to bundling js/css assets in this PR. By importing e.g. import 'bootstrap';, the bootstrap dependency is bundled. Same idea goes for css.

You took the CopyPlugin approach which is also fine. One downside about this approach is that we define dependencies in the webpack configuration step, defining dependencies where they are used feels more correct?
An extra benefit of the import and let-Webpack-bundle-it approach is that we can bundle common chunks into a second vendor output. These outputs can then be fingerprinted.

I think upstreaming #167 would be a good first step. Splitting common chunks can be a different PR. Or maybe you prefer the CopyPlugin setup?

[bollwyvl]

I've merged #167 (by way of master).

Or maybe you prefer the CopyPlugin setup?

this currently keeps the copy step for the fonts (fontawesome, lato, open sans). As to fingerprinting: i don't know what particular method you're using for that, does it occur in CI somewhere? Anyhow, the fonts are still 403 on the CI deploy.

To use another approach for the fonts, it would take at least one more loader/plugin (there are a few to choose from), and a stack of config, but again, there's a lot of conflicting info out there, and I don't care to keep fighting with it.

mathjax will steadfastly resist being webpacked, as its model is highly dynamic, and heavily relies on relative paths to address different fonts/plugins.

given this, and that someone has already packaged it, perhaps this concern should be delegated to a dependency on py-mathjax and config settings... or just documentation

the demo now brings in require.js and the widgets bundles from CDN... fixing this seems out of scope, at present, as there's nothing directly controlling them in this repo.
However, if the intent is to support jupyter_sphinx more directly, the requirejs file could be included in the vendor bundle, and detected directly

i guess it depends on how committed this theme is to offering a low-config way to achieving an offline mode.

[bollwyvl]

Actually, it looks like mathjax 3 can in fact be webpacked... http://docs.mathjax.org/en/latest/web/webpack.html

[hoetmaaiers]

I understand the struggle we look upon when bundling fonts via Webpack. It is indeed possible, but would toke some time and headache. Since this PR provides a good alternative via the CopyPlugin, I am glad to approve this plugin.

Good to know Mathjax is webpack-able.

My idea of fingerprinting is to provide a hash for every bundled output asset. This is already configured in this project: https://github.com/pandas-dev/pydata-sphinx-theme/blob/master/webpack.common.js#L10 using the output [hash] template: https://webpack.js.org/configuration/output/.

[hoetmaaiers]

Looked over this, font awesome on the CI build should work before we can merge.

[jorisvandenbossche]

Some more questions about this:

• I see this now needs a yarn build step in CI. Does this mean that also the user, when installing from the git repo, will always need yarn? (so pip install from the git repo will not work anymore?) And I suppose this is because now the "vendored" css/js needs to be copied, and we don't include that "pre-built" in the actual repo?
  Personally, I find this a downside, as I would like to keep it as accessible as possible for python people to quickly try out (of course, once you want to edit something in the css, you will need yarn anyway).

• There are "Access-Control-Allow-Origin" errors in the console (https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS/Errors/CORSMissingAllowOrigin). That might be related to why fontawesome is not yet working, although I also see logs like that about open-sanse

• The MathJax rendering changed. I first thought that is maybe because of the actual config being used (config=TeX-AMS-MML_HTMLorMML), but when loading a page in the preview site that has math (eg demo/demo.html), there are also some errors about mathjax in the console (eg about MathJax_Main font missing, possible the same issue as the item above)

[jorisvandenbossche]

the demo now brings in require.js and the widgets bundles from CDN... fixing this seems out of scope, at present, as there's nothing directly controlling them in this repo.
However, if the intent is to support jupyter_sphinx more directly, the requirejs file could be included in the vendor bundle, and detected directly

@bollwyvl to clarify this: I don't think we necessarily want to support jupyter_sphinx directly, but we do want that users of this theme can use such sphinx extensions (but that maybe amounts to the same thing in the end ..).
So the main reason jupyter_sphinx is included in the demo site, is as an example test case, to ensure the theme actually works with other extensions that use requirejs (which wasn't the case before).

[hoetmaaiers]

The font loading issue seems to be Circle CI related and also reported over here: https://discuss.circleci.com/t/circleci-403-forbidden-in-html-artifacts/35791.

[jorisvandenbossche]

The font loading issue seems to be Circle CI related and also reported over here: https://discuss.circleci.com/t/circleci-403-forbidden-in-html-artifacts/35791.

Ah, indeed (the Eric reporting it there is actually the person from whom we copied the approach to preview the artifacts on CircleCI).
I tested the PR locally, and there it is indeed working fine.

But, MathJax still has the same problem locally:

Loading failed for the <script> with source “file:///home/joris/scipy/repos/pydata-sphinx-theme/docs/_build/html/_static/vendor/mathjax/fonts/HTML-CSS/TeX/png/imagedata.js?V=2.7.5”. demo.html:1:1
TypeError: c.FONTDATA.FONTS.MathJax_Main[8212][5] is undefinedimage Fonts.js:19:3651

[bollwyvl]

apologies for the delay on replying:

Looked over this, font awesome on the CI build should work before we can merge.

Yeah, debugging circle is probably out of my ability to fix! No doubt this would also occur with the webpack-all-the-things approach.

Does this mean that also the user, when installing from the git repo, will always need yarn?

yes. after this PR, these assets could be checked in, but while I'm tinkering, there's a fair amount of thrashing. I've handled this in similar cases by submoduling these kinds of things, but i have little experience with pip installing from git, and don't know if that would be viable.

I don't think we necessarily want to support jupyter_sphinx directly

Sure, sounds fine: continuing down this line of thought, it may not even be the job of this theme to even handle mathjax, as that's a "blessed" sphinx extension. likely there is/should be another extension, e.g. sphinx-local-mathjax that uses py-mathjax. So i can just stop doing it.

But, MathJax still has the same problem locally:

that's odd, hadn't seen those locally. will look again!

[bollwyvl]

Yeah, can't reproduce the MathJax issue locally:

[hoetmaaiers]

@bollwyvl with the workflow written out, it makes more sense to me. Also the comment in webpack-macros.html: "these macros are generated by "yarn build". do not edit by hand." now clicks.

Would it make sense to document the workflow somewhere?

Thank you for the work!

[choldgraf]

Hey all - just a note that I just cut a new patch release (https://github.com/pandas-dev/pydata-sphinx-theme/releases/tag/v0.3.1) since we had made some small improvements there, and since I suspect we'll want some time to try out this PR once it's merged before a release.

[bollwyvl]

I took a stab at updating contributing.rst, and a very minimal section in customizing.rst.

Along the way i found i had to:

• move webpack-macros.html into static
  • otherwise CleanPlugin could leave the files in a bad state
• update webpack.dev.js to writeToDisk
  • otherwise it couldn't find the macros (which also needed to be ignored from watching)
• update the pull request screenshot, as you're gonna have to scroll now... maybe there's a way to rename them to be a bit more friendly on a future PR, and push it to the top of the list... or better still, post a descriptive comment back.

About the only thing left discussed above would be to tighten up the font stuff to be driven off some bespoke data in package.json, but I think the now-documented steps aren't that odious, given the examples that are already in webpack.common.js.

Otherwise, I'm hitting diminishing marginal returns on successive re-reads, so am probably done, modulo review.

[jorisvandenbossche]

@bollwyvl thanks a lot for the updates to the contribution guide!

I am away for a couple of days, and could only review afterwards. So I am also fine with merging now so it can sit in master. And we can still further work on some issues if there are left in follow-up PRs.

[bollwyvl]

Your call: I'd say push it off... If people really are using master, this is a fairly substantial change and might require quick action if I screwed something up badly!

…

[choldgraf]

I'm fine either way - as I said, I just pushed a new release yesterday in anticipation of this PR getting merged :-)

[jorisvandenbossche]

@bollwyvl thanks a lot for the detailed docs (and update of the existing docs)

Let's give this a try! (finally .. sorry it took so long)

[choldgraf]

woooooo 🎉🎉🎉🎉🎉

[bollwyvl]

Thanks, all, for helping get this over the finish line!

[jorisvandenbossche]

One more question, not directly related to this PR but that I was wondering while reviewing this earlier today: why do we also vendor the Open Sans font? Because from our css it seems we only include the Lato one?

[jorisvandenbossche]

Whoops, ignore me, I see we use it for the header-style