Update node packages and remove unneeded ones

[drammock]

closes #1257

This PR bumps our JS requirements as follows:

• copy-webpack-plugin 5.1.1 -> 11.0.0
• node-sass 7.0.3 -> 8.0.0
• webpack 4.41.6 -> 5.0.0
• webpack-cli 3.3.11 -> 5.0.0

The first two were done by npm audit fix --force. The latter two I did manually to resolve dependency resolution issues. A slight change to our webpack config file was also needed.

Locally at least, I can get nox -s docs to work, and nox -s docs-live works (modulo the problem in #1256, which sadly this doesn't fix). Let's see if the CIs still succeed.

Would appreciate a review from someone who is not fumbling blindly when it comes to JS/node/webpack... the fact that this seems to have worked (?) is a testament to the fact that reading tracebacks is apparently a (python -> JS) transferrable skill. @choldgraf are you that person, or can you tag someone who is?

EDIT this PR also removes several modules that were determined to be no longer necessary for our build chain (if they ever were).

[trallard]

Considering this is JS heavy - @gabalafou could you spare a few mins and help us with reviewing this?

[drammock]

Considering this is JS heavy - @gabalafou could you spare a few mins and help us with reviewing this?

that would be awesome. Also we could add axe-core here since it will be needed for #1260, just to get all the JS dependency updates done all at once. WDYT?

[gabalafou]

In my humble opinion, it's better for future contributors if the axe-core dependency comes in with the code that actually uses it. Putting myself in the shoes of a future contributor, if I did a git blame package.json, and it led me to this PR, I would think that axe-core was introduced before this PR (since the title of this PR has "update" in it), which would lead me to go looking in vain for when it was first introduced. Yes, it's true, that doesn't take into account this conversation that we're having right now on this PR, but still, I think it's better not to add axe-core in this PR.

[drammock]

it's better for future contributors if the axe-core dependency comes in with the code that actually uses it.

Fair enough.

if I did a git blame package.json, and it led me to this PR, I would think that axe-core was introduced before this PR (since the title of this PR has "update" in it)

I mean, we can change the title of this PR 🙂 but I think your earlier point stands. Which I guess means this is still ready for review.

[gabalafou]

So this might seem extreme, but I might go a step further and remove all unused dependencies.

The reason why is that if you don't remove these unused dependencies then you're stuck having to continually update them whenever you do an automated audit of the list of dependencies for ones that are outdated or have security vulnerabilities (unfortunately, there's no automated audit for unused dependencies). What led me to this conclusion was that while reviewing this PR, I could not find anywhere that node-sass was being used. At first I thought maybe it was added by Dependabot, but when I stepped through the history of package.json it led me all the way back to when the file was first added. Maybe earlier versions of sass-loader required it as a peer dependency? I have no idea but you can easily see that it's now a direct dependency of our version of sass-loader if you run npm ls --depth 1 after syncing the lock file with my updates. (Note that you'll see some warnings when you run the npm ls --depth 1 command. That's because optimize-css-assets-webpack-plugin is not supported for webpack v5 and up.)

The only thing is, I'm not sure exactly how to test for hidden dependencies because I'm not entirely sure how stb (Sphinx Theme Builder) works under the hood. But I was able to run all of the manual dev environment commands (install, build, compile, serve, test) without any problems (except the infinite rebuild loop with stb serve docs that we already know about). And I was able to build and serve the docs for PyData Sphinx Theme on my machine. I was able to visit them at localhost:8000 in my browser, I clicked on several pages, couldn't spot any problems, but I don't know exactly which pages in the docs site to check to make sure nothing was broken.

[drammock]

I might go a step further and remove all unused dependencies.

I'm fine with that. I honestly don't know why most of those are in here, other than a vague "probably because webpack which I haven't taken the time to really understand".

The only thing is, I'm not sure exactly how to test for hidden dependencies because I'm not entirely sure how stb (Sphinx Theme Builder) works under the hood.

me neither 🙂 But I'm reasonably sure that if you removed something and nox -s compile still works, then we didn't need it. I'll merge in your changes here.

[gabalafou]

@drammock nice! Could you update the PR title and description?

[gabalafou]

My review: looks good to me. I also tested with nox -s compile and didn't get any issues.

I hesitate to click the "Approve" button simply because a change this big should probably get a green light from someone with more experience working in this codebase.

[drammock]

OK @gabalafou I could use your input again. I've updated the CSS minimizer and updated the webpack config accordingly, fixed a "cannot load sass" error, and fixed a couple of SASS compiler errors, but I'm still getting failures in nox -s compile to the tune of

Error: Unexpected '/'. Escaping special characters with \ may help.

AFAICT these are errors coming from the minimizer when trying to minimize the already-compiled-from-SCSS CSS files, and in the 2 files it complains about the / character only occurs in comment lines. Probably there's a flag missing from either the SCSS compilation step or the CSS Minimizer step that will strip or ignore comments... is it obvious to you what change is needed here? If not I can probably figure it out eventually with some trial and error.

EDIT here's the full traceback:

      ERROR in styles/bootstrap.css
      styles/bootstrap.css from Css Minimizer plugin
      Error: Unexpected '/'. Escaping special characters with \ may help.
          at /opt/pydata-sphinx-theme/styles/bootstrap.css:1:1
          at Root._error (/opt/pydata-sphinx-theme/node_modules/postcss-selector-parser/dist/parser.js:174:16)
          at Root.error (/opt/pydata-sphinx-theme/node_modules/postcss-selector-parser/dist/selectors/root.js:43:19)
          at Parser.error (/opt/pydata-sphinx-theme/node_modules/postcss-selector-parser/dist/parser.js:740:21)
          at Parser.unexpected (/opt/pydata-sphinx-theme/node_modules/postcss-selector-parser/dist/parser.js:758:17)
          at Parser.combinator (/opt/pydata-sphinx-theme/node_modules/postcss-selector-parser/dist/parser.js:656:12)
          at Parser.parse (/opt/pydata-sphinx-theme/node_modules/postcss-selector-parser/dist/parser.js:1101:14)
          at Parser.loop (/opt/pydata-sphinx-theme/node_modules/postcss-selector-parser/dist/parser.js:1043:12)
          at new Parser (/opt/pydata-sphinx-theme/node_modules/postcss-selector-parser/dist/parser.js:164:10)
          at Processor._root (/opt/pydata-sphinx-theme/node_modules/postcss-selector-parser/dist/processor.js:53:18)
          at Processor._runSync (/opt/pydata-sphinx-theme/node_modules/postcss-selector-parser/dist/processor.js:100:21)
      
      ERROR in styles/pydata-sphinx-theme.css
      styles/pydata-sphinx-theme.css from Css Minimizer plugin
      Error: Unexpected '/'. Escaping special characters with \ may help.
          at /opt/pydata-sphinx-theme/styles/pydata-sphinx-theme.css:1:1
          at Root._error (/opt/pydata-sphinx-theme/node_modules/postcss-selector-parser/dist/parser.js:174:16)
          at Root.error (/opt/pydata-sphinx-theme/node_modules/postcss-selector-parser/dist/selectors/root.js:43:19)
          at Parser.error (/opt/pydata-sphinx-theme/node_modules/postcss-selector-parser/dist/parser.js:740:21)
          at Parser.unexpected (/opt/pydata-sphinx-theme/node_modules/postcss-selector-parser/dist/parser.js:758:17)
          at Parser.combinator (/opt/pydata-sphinx-theme/node_modules/postcss-selector-parser/dist/parser.js:656:12)
          at Parser.parse (/opt/pydata-sphinx-theme/node_modules/postcss-selector-parser/dist/parser.js:1101:14)
          at Parser.loop (/opt/pydata-sphinx-theme/node_modules/postcss-selector-parser/dist/parser.js:1043:12)
          at new Parser (/opt/pydata-sphinx-theme/node_modules/postcss-selector-parser/dist/parser.js:164:10)
          at Processor._root (/opt/pydata-sphinx-theme/node_modules/postcss-selector-parser/dist/processor.js:53:18)
          at Processor._runSync (/opt/pydata-sphinx-theme/node_modules/postcss-selector-parser/dist/processor.js:100:21)
      
      webpack 5.76.3 compiled with 2 errors in 5882 ms

[gabalafou]

Give my following changes a try?

• remove file loader and other unused drammock/pydata-sphinx-theme#2

To be perfectly honest, I don't really know what I'm doing with Webpack half the time. I always find myself shuffling lines of code until the configuration file magically seems to work and everything just builds. But that's not really how you want to go about setting up your build system. You want to know what each part is doing and why—what each step takes in as input and what each step creates as output.

All I can say is that my proposed change allows the stb compile command to work, and when I build the docs locally with nox -s docs, they look fine to me (but again, I don't know where specifically to look in the docs site for signs that something might have been built incorrectly). Without very careful examination, then for all I know, these changes are, for example, causing the assets to be bundled in some really badly sub-optimal way.

The problem is that I would need to have an intimate understanding of the entire build process for what this repo is trying to output, and I just haven't connected all of those dots yet. If there's someone in this community who has the full picture of the build process and wants to go through it with me, then I can weigh in with more confidence. Otherwise, I would have to spend hours reverse engineering all of this, which is not something I have the time for right now.

All I can say right is that the doc site looks alright. There are a number of errors in the console logs, including 404s on font files, but I find many of the same errors on the Read The Docs site, so 🤷

[drammock]

There are a number of errors in the console logs, including 404s on font files, but I find many of the same errors on the Read The Docs site, so shrug

those have been bugging me for a long time but I haven't had the bandwidth to fix them 😞

[drammock]

well, we broke something: https://pydata-sphinx-theme--1258.org.readthedocs.build/en/1258/#

[gabalafou]

Yay! 😁

Let's see what happens if we sub sass with node-sass

[gabalafou]

The sass-loader docs highly recommend using Dart Sass (npm i sass) over Node Sass (npm i node-sass) so maybe we should stick with that and see if everything still builds.

[drammock]

Let's see what happens if we sub sass with node-sass

Locally, using the old spec of node-sass ^7.0.3 works but has 2 moderate severity vulnerabilities. Bumping to node-sass ^8.0.0 (as npm audit --force would do) leads to build errors

ERROR in ./src/pydata_sphinx_theme/assets/styles/pydata-sphinx-theme.scss (./src/pydata_sphinx_theme/assets/styles/pydata-sphinx-theme.scss.webpack[javascript/auto]!=!./node_modules/css-loader/dist/cjs.js?-url!./node_modules/sass-loader/dist/cjs.js??ruleSet[1].rules[0].use[2]!./src/pydata_sphinx_theme/assets/styles/pydata-sphinx-theme.scss)
      Module build failed (from ./node_modules/sass-loader/dist/cjs.js):
      SassError: Invalid CSS after "... map-get(pstcol": expected expression (e.g. 1px, bold), was ".$pst-semantic-colo"
              on line 53 of src/pydata_sphinx_theme/assets/styles/extensions/_sphinx_design.scss
              from line 72 of src/pydata_sphinx_theme/assets/styles/pydata-sphinx-theme.scss
      >>   "dark": map-get(pstcol.$pst-semantic-colors, "text-base"),

plus the main site for node-sass is encouraging people not to use it and to use sass instead, so if we can get it to work with sass I think we should. Looks like your suggestion did the trick too: https://pydata-sphinx-theme--1258.org.readthedocs.build/en/1258/

[gabalafou]

Yeah my original hypothesis for the failing build was because I read that node-sass provides bindings to a C++ library, so I thought maybe the Read the Docs build was relying on that or something. But then I noticed that when I removed the file-loader plug-in, it was providing a specific output file pattern that I forgot to provide to the remaining plugins and that would explain why the styles were not loading.

[drammock]

@12rambau @choldgraf this one is ready for merge IMO. Thanks 💯 ** 3 @gabalafou for your help here!

[trallard]

Note: not blocking - that this will change with the changes in #1174 as I encountered issues with using * while working on this

[trallard]

If someone in this community has the complete picture of the build process and wants to go through it with me, then I can weigh in with more confidence.

My best guess on who these people are would be @choldgraf or @pradyunsg

Also yay for now using dart sass 🎉 - it will help a lot with some of the improvements planned (from the a11y work) thanks @drammock and @gabalafou

[choldgraf]

You should definitely not treat me as an expert on webpack or javascript, I am fumbling in the dark just as much as y'all are. But this seems reasonable to me and I'm +1

[12rambau]

As everyone here, I don't have a very sharp eye on webpack changes. My guess is as long as tests and rendering are happy, I'm happy as well.