UPDATE: NodeJS 16.x

[choldgraf]

This updates our NodeJS version to the latest LTS release.

@pradyunsg I wasn't sure if sphinx-theme-builder allows you to specify something like node-version = 16.x so I just pinned the full version, but if you tell me that it is possible then I'm happy to make a quick docs PR to note this in the STB

closes #551 closes #537

[damianavila]

I just pinned the full version

I always prefer to be explicit about the version being used.

[damianavila]

With this PR I noticed we do not have visibility of what is happening on the stb side of things...
And I am actually surprised I do not see any stb compile in the workflow file... should we add something like that to test the compilation process somehow?

[choldgraf]

We do have it in our noxfile (https://github.com/pydata/pydata-sphinx-theme/blob/master/noxfile.py#L35), and I believe that the build step in our "publish to PyPI" job does this as well, but we should double check and document (here and or upstream to STB!)

Can we capture that in a new issue to tackle as part of the dev workflow improvements?

[pradyunsg]

You likely need to regenerate package-lock.json and check that in too!

And I am actually surprised I do not see any stb compile in the workflow file...

It will be run as part of:

pydata-sphinx-theme/.github/workflows/tests.yml

Line 46 in 7d64c92

python -m pip install -e .[coverage]

The whole design is that the NodeJS pipeline is fetched and used "transparently" as part of the wheel generation process.

[pradyunsg]

Wheel generation calling generate_assets:

https://github.com/pradyunsg/sphinx-theme-builder/blob/10450cfeb65bc3610a6708ee731069c707978fef/src/sphinx_theme_builder/_internal/distributions.py#L157

stb compile calling generate_assets:

https://github.com/pradyunsg/sphinx-theme-builder/blob/10450cfeb65bc3610a6708ee731069c707978fef/src/sphinx_theme_builder/_internal/cli/compile.py#L26

[damianavila]

Can we capture that in a new issue to tackle as part of the dev workflow improvements?

Sure thing!

The whole design is that the NodeJS pipeline is fetched and used "transparently" as part of the wheel generation process.

Yep, and you are doing a great job on that direction @pradyunsg, but as a dev for the theme, I would like to see what is happening under the hood in the CI as well 😜 ... this is why I think we should explicitly add a stb compile step in the future.

[choldgraf]

@pradyunsg you said:

You likely need to regenerate package-lock.json and check that in too!

But this file doesn't exist anywhere in the repository. If I re-run stb compile (after deleting .nodeenv so that it grabs a new NPM), it builds fine (with some warnings), but no new files are generated anywhere. Could you provide guidance on where to look for package-lock.json?

From @damianavila:

I would like to see what is happening under the hood in the CI as well

Would it be enough to improve the documentation in the sphinx-theme-builder about what happens when python -m build is invoked, as well as what happens when stb compile is called, and then add a section in our documentation that links out to those more detailed sections?

[jorisvandenbossche]

But this file doesn't exist anywhere in the repository

We do have https://github.com/pydata/pydata-sphinx-theme/blob/master/package.json (without the "lock" part). It seems that furo has checked in both files.

[jorisvandenbossche]

I would like to see what is happening under the hood in the CI as well

We could also make the install step in CI more verbose? (so that the pip install step shows the output from stb?)

[pradyunsg]

I think adding a -v to pip is a good idea!

[pradyunsg]

But this file doesn't exist anywhere in the repository

Ah, interesting! Never mind me then!

[pradyunsg]

Assuming your dependencies still work, this LGTM!

[pradyunsg]

I wasn't sure if sphinx-theme-builder allows you to specify something like node-version = 16.x

It doesn’t. That’s a limitation inherited from nodeenv.

[damianavila]

If I re-run stb compile (after deleting .nodeenv so that it grabs a new NPM), it builds fine (with some warnings)

@choldgraf, warnings about?

Would it be enough to improve the documentation in the sphinx-theme-builder about what happens when python -m build is invoked, as well as what happens when stb compile is called, and then add a section in our documentation that links out to those more detailed sections?

Docs are always welcome, but I still feel we need visibility on what is happening under the hood when we run the CI job.

We do have https://github.com/pydata/pydata-sphinx-theme/blob/master/package.json (without the "lock" part). It seems that furo has checked in both files.

Yep, AFAIK, the lock file is intended to be included in the repo: https://docs.npmjs.com/cli/v8/configuring-npm/package-lock-json

We could also make the install step in CI more verbose? (so that the pip install step shows the output from stb?)

+1, if that allows us to see a piece of the compilation stuff, I will be more than happy 😉

[pradyunsg]

stb compile is basically running the same code as python -m build (although, the build also generates a wheel with all the stuff). See #552 (comment).

[choldgraf]

@damianavila the warnings were just things like deprecated dependencies, it didn't seem to be anything too bad. I'll put a warning log at the bottom of this comment and maybe we should open an issue to track updating those dependencies?

re: being verbose, I agree but I can't figure out how to make stb compile or python -m build be verbose - neither of them have --verbose or -v arguments as far as I can tell. Can we push that to a new issue to track rather than blocking this PR on it?

for the lockfilee, I agree it seems like this should be checked into the repository...but I don't know why it isn't generated in the first place. Maybe something to do with our webpack config? For this as well, can we track it in an issue or do you think this should block upgrading node?

My vote is that we do the following:

1. convince ourselves that NodeJS16 doesn't introduce bugs for us
2. merge this PR
3. Open issues for: "more verbosity in our compile step, especially in our publishing build", "update deprecated dependencies", and "add a package lockfile to the repo".

Warnings generated by webpack

npm WARN deprecated source-map-url@0.4.1: See https://github.com/lydell/source-map-url#deprecated
npm WARN deprecated urix@0.1.0: Please see https://github.com/lydell/urix#deprecated
npm WARN deprecated har-validator@5.1.5: this library is no longer supported
npm WARN deprecated resolve-url@0.2.1: https://github.com/lydell/resolve-url#deprecated
npm WARN deprecated source-map-resolve@0.5.3: See https://github.com/lydell/source-map-resolve#deprecated
npm WARN deprecated chokidar@2.1.8: Chokidar 2 will break on node v14+. Upgrade to chokidar 3 with 15x less dependencies.
npm WARN deprecated chokidar@2.1.8: Chokidar 2 will break on node v14+. Upgrade to chokidar 3 with 15x less dependencies.
npm WARN deprecated querystring@0.2.0: The querystring API is considered Legacy. new code should use the URLSearchParams API instead.
npm WARN deprecated mkdirp@0.3.0: 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 uuid@3.4.0: 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 request@2.88.2: request has been deprecated, see https://github.com/request/request/issues/3142
npm WARN deprecated svgo@1.3.2: This SVGO version is no longer supported. Upgrade to v2.x.x.
npm WARN deprecated popper.js@1.16.1: You can find the new Popper v2 at @popperjs/core, this package is dedicated to the legacy v1
npm WARN deprecated core-js@2.6.12: 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.

[jorisvandenbossche]

My vote is that we do the following:

+1, from a quick look at the readthedocs preview, everything looks fine (I also would not expect changes, as node itself shouldn't impact the built site if we don't change any version of actual package we are using). I agree the other issues are not strictly related to upgrading the node version.

[choldgraf]

OK I've done some investigating and opened up 3 new issues:

• Add a package-lock.json for dependencies #557
• Update dependencies that are creating warnings #558
• Create more verbosity in our stb compile step for publishing the package #559

Can we merge this one in so that we resolve the dev installation issues on Mac, and then iterate on those next?