Fix a number of issues with Webpack

[agjohnson]

A few of these squeezed by QA and testing:

• badge_only.css was copied to the static output path, it wasn't
  actually building it seems. Added second entry for this to make it
  easier.
• jQuery was not being treated as an external library and was vendored
  into our JS bundle!
• Having the fonts in a relative path from CSS ('../fonts'), causes CORS
  issues. Moved /fonts to /sphinx_rtd_theme/static/css/fonts essentially. See Fonts are different between local html and readthedocs.org  #782
• The dev server wasn't compiling assets consistently for me, tuned it hopefully
• This does output an unused js/badge_only.js, which I believe is for using CSS through JS. This is a webpack 4 thing and it can't be removed. I say disregard it for now.

Requires #807, based on that until it's merged, then can be repointed to master
Refs #782

[jessetan]

My build cannot find fontawesome:

ERROR in ./src/sass/theme.sass
Module build failed (from ./node_modules/mini-css-extract-plugin/dist/loader.js):
ModuleNotFoundError: Module not found: Error: Can't resolve '../fonts/fontawesome-webfont.eot' in '/Users/jesse/Documents/development/sphinx_rtd_theme/src/sass'

This looks similar to an issue with tilde in $fa-font-path and sass-loader reported in webpack-contrib/sass-loader#566

If I downgrade sass-loader to something <7, like 6.0.7 it works.
This is an issue in master as well.

[agjohnson]

Hrm odd. I'll poke this on my environment to see what I find.

[jessetan]

Looks like it is not unique to my setup: #820

[jessetan]

@agjohnson how is the progress on this? It is one of the things that is holding back a release.

[agjohnson]

I haven't had time in Sep to focus on the theme, but have some changes on our roadmap that I'm assigned to. I'll be getting back to some of this work in a week or so. If you find a fix, feel free to jump in -- I still have not tried to reproduce the error yet even, so I'm not sure what I'll hit yet.

[jessetan]

Thanks @agjohnson, sounds good.
I have no explanation why we have different results, but the issue with the font awesome path is resolved for me if I use "sass-loader": "^6.0.7"

[davidfischer]

badge_only.css was copied to the static output path, it wasn't actually building it seems. Added second entry for this to make it easier.
jQuery was not being treated as an external library and was vendored into our JS bundle!

These look resolved to me

I have no explanation why we have different results, but the issue with the font awesome path is resolved for me if I use "sass-loader": "^6.0.7"

I don't have this issue and the configuration worked as-is for me.

Moved /fonts to /sphinx_rtd_theme/static/css/fonts

The fonts "work" but they are duplicated. More or less the same fonts exist in sphinx_rtd_theme/static/fonts/ and sphinx_rtd_theme/static/css/fonts/. Both of these are copied to the docs output when running npm run dev or to the module bundle when running python setup.py sdist.

sphinx_rtd_theme/static/fonts/ does not appear to be used at all and could probably just be deleted.

sphinx_rtd_theme/static/css/fonts/ is created and populated by npm run build. Its could probably be removed and the directory itself added to .gitignore.

[pedro-w]

Just to add to this I get the same error as @jessetan when running these commands:

git clone https://github.com/readthedocs/sphinx_rtd_theme.git
git checkout agj/more-webpack-fixes
pip3 install -e '.[dev]'
npm install
npm run dev

@davidfischer is this what you are doing or is there another way to do it?

[edit:] on Debian 10.1, Python 3.7.3, NPM 6.9.0
Also, sass-loader version 6 fixes it.

[jessetan]

@davidfischer perhaps it is different environments. I use a pipenv shell on macOS 10.13.6 with npm 6.12.0, Python 3.7.3. The error appears when I run npm run dev right before Sphinx is started.

[pedro-w]

I looked a bit more into sass-loader versions

• 6.x OK
• 7.0.x OK
• 7.1.0 OK
• 7.2.0 Error as above
• 7.3.x Error as above

Don't know if that helps?

[davidfischer]

7.1.0 OK

This is the version that I had installed so that may explain things.

I had a package-lock.json file which is currently gitignored and was pinned to 7.1.0. We should probably commit that file and pin to a specific version. That seems to be what npm suggests:

npm notice created a lockfile as package-lock.json. You should commit this file.

[davidfischer]

I created a PR that could merge into this one that commits the package-lock.json and pins sass-loader to 7.1: #827

[pedro-w]

I am out of my depth on all this npm stuff, but it would be great if someone could figure out what causes the problem (since sass-loader 8 is already out). I had a further poke around and one thing I noticed is that the version of the css-loader dependency jumps from 0.28 to 2.0 between these two versions. Maybe @evilebottnawi might know.

[alexander-akait]

@pedro-w what is problem?

[pedro-w]

@evilebottnawi thanks for the quick response. If you look at this comment the problem is that sphinx_rtd_theme's build system can't resolve the fontawesome fonts. Further, it seems that with sass-loader v7.1.0 and before it works, and v7.2.0 and later does not work. So I was hoping you would know straight away if were there any changes to the resolving code in between those two versions that could cause this? Or, has anything like this come up before? Thanks!

[alexander-akait]

@pedro-w maybe you can create minimum reproducible test repo and open issue in sass-loader?

[pedro-w]

I believe if you change the import here

sphinx_rtd_theme/src/sass/theme.sass

Line 31 in ddf840c

@import font-awesome

to @import ~font-awesome/scss/font-awesome then it will solve the issues with sass-loader versions (7.1 and earlier vs 7.2 and later)

[davidfischer]

I created a small PR that merges into this one based on your suggestion. It worked for me and I'd be happy to be able to take a later version. #830

[agjohnson]

Looking through some of the remaining comments, there are already some fixes in master that are not in this branch.

sphinx_rtd_theme/static/css/fonts/ is created and populated by npm run build. Its could probably be removed and the directory itself added to .gitignore.

master has the fix for this. I'll try rebasing this branch, but i don't get the static/fonts path generated on master + this branch.

If there is any feedback or if anyone else wants to get review in, I'd like to get this merged soon.

[davidfischer]

I think we accidentally merged this into agj/format-webpack-js rather than master. Shall I revert it and repoint at master?

[agjohnson]

Should be merged manually now