pydata-sphinx-theme icon indicating copy to clipboard operation
pydata-sphinx-theme copied to clipboard

Update dependencies that are creating warnings

Open choldgraf opened this issue 3 years ago • 7 comments

Description

We have not updated our dependency chain in package.json in a while. As a result, when we do a fresh npm install, we get a bunch of warnings about package vulnerabilities and such. This probably isn't a huge problem, because most of these are not bundled with the theme itself, but it would be good to do a quick audit and figure out which ones we can quickly update to remove the warning.

Guide

To generate the warnings, do a fresh install of this theme locally (if already installed, just delete the nodeenv folder) and re-run stb compile. This should generate several warnings.

To do a deeper dive of the warnings, you can manually run an audit via stb npm audit.

Then we need to update the dependencies accordingly in package.json.

Warnings generated by webpack
npm WARN deprecated [email protected]: See https://github.com/lydell/source-map-url#deprecated
npm WARN deprecated [email protected]: Please see https://github.com/lydell/urix#deprecated
npm WARN deprecated [email protected]: this library is no longer supported
npm WARN deprecated [email protected]: https://github.com/lydell/resolve-url#deprecated
npm WARN deprecated [email protected]: See https://github.com/lydell/source-map-resolve#deprecated
npm WARN deprecated [email protected]: Chokidar 2 will break on node v14+. Upgrade to chokidar 3 with 15x less dependencies.
npm WARN deprecated [email protected]: Chokidar 2 will break on node v14+. Upgrade to chokidar 3 with 15x less dependencies.
npm WARN deprecated [email protected]: The querystring API is considered Legacy. new code should use the URLSearchParams API instead.
npm WARN deprecated [email protected]: Legacy versions of mkdirp are no longer supported. Please update to mkdirp 1.x. (Note that the API surface has changed to use Promises in 1.x.)
npm WARN deprecated [email protected]: Please upgrade  to version 7 or higher.  Older versions may use Math.random() in certain circumstances, which is known to be problematic.  See https://v8.dev/blog/math-random for details.
npm WARN deprecated [email protected]: request has been deprecated, see https://github.com/request/request/issues/3142
npm WARN deprecated [email protected]: This SVGO version is no longer supported. Upgrade to v2.x.x.
npm WARN deprecated [email protected]: You can find the new Popper v2 at @popperjs/core, this package is dedicated to the legacy v1
npm WARN deprecated [email protected]: core-js@<3.4 is no longer maintained and not recommended for usage due to the number of issues. Because of the V8 engine whims, feature detection in old core-js versions could cause a slowdown up to 100x even if nothing is polyfilled. Please, upgrade your dependencies to the actual version of core-js.

choldgraf avatar Jan 14 '22 17:01 choldgraf

At some point in the past, we got dependabot PRs to update javascript dependencies (eg https://github.com/pydata/pydata-sphinx-theme/pull/443). But it seems those PRs have stopped the last months.

jorisvandenbossche avatar Jan 14 '22 18:01 jorisvandenbossche

I found dependabot super annoying in other projects... just saying 😜

damianavila avatar Jan 18 '22 15:01 damianavila

Can you explain a bit more what aspect you found annoying? (too noisy / too many small PRs?)

jorisvandenbossche avatar Jan 18 '22 16:01 jorisvandenbossche

In my experience the biggest challenges with dependabot are that it lowers the signal to noise in the pull requests - you get a lot of opened PRs for patch/minor versions, and almost none of those PRs actually impact the functionality of the tools you're using. If you define really clear team policies about aggressively merging dependabot PRs, maybe it's not as bad, but in my experience you end up with like 5 dangling dependabot PRs that haven't been touched by anyone.

choldgraf avatar Jan 18 '22 17:01 choldgraf

Yes, I can relate to that :) (with the limited experience on this theme)

It would be nicer if you could manually trigger dependabot to create one PR to update a bunch of packages every few months or so.

jorisvandenbossche avatar Jan 18 '22 20:01 jorisvandenbossche

Looks like maybe we could configure it to only check once every so often:

https://docs.github.com/en/code-security/supply-chain-security/keeping-your-dependencies-updated-automatically/configuration-options-for-dependency-updates

choldgraf avatar Jan 18 '22 21:01 choldgraf

Can you explain a bit more what aspect you found annoying? (too noisy / too many small PRs?)

I have also seen cases where people, just annoyed about the pings, merge any dependabot PR without any actual checking. Given that we have a theme here, I am worried induced-quickly merging the dependabot PRs are going to break things that we are not testing (for instance, things that need visual checking).

Reducing the check time as you both suggested might help to reduce the induced "bad" behavior.

damianavila avatar Jan 25 '22 22:01 damianavila

I recently saw some dependabot PR being merged, should we close this issue or is the configuration of dependabot still on the table ?

12rambau avatar Sep 26 '22 16:09 12rambau

+1 to close, and to have a policy of doing what dependabot suggests promptly (I'd rather deal with it occasionally breaking something visual that we don't notice right away, rather than having security vulnerabilities lurking that I don't understand the severity or consequences of).

drammock avatar Sep 26 '22 19:09 drammock