Swap to using webpack #771

SimonBiggs · 2019-07-02T10:19:23Z

This is pull request is a follow up to the following issue:

The live reload server is run with yarn start, the production build is built with yarn build.

src/sass/_theme_font_local.sass

jessetan

There are a lot of font files in sphinx_rtd_theme/static/fonts. Do you need them if they are installed as dependencies?

package.json

SimonBiggs · 2019-07-04T20:32:03Z

There are a lot of font files in sphinx_rtd_theme/static/fonts. Do you need them if they are installed as dependencies?

Given those dependencies won't be shipped in pypi I would say yes? Webpack is actually outputting those files from the dependencies when it runs the build.

The number of font files could be reduced by scoping Roboto as well. Couldn't work out how to do that though.

jessetan · 2019-07-05T08:05:34Z

I can not get the dev server to work unfortunately.
If I run yarn build I get a freshly compiled theme.js in sphinx_rtd_theme/static/js.
If I run yarn start I see that theme.js is recompiled every time I make a change, but the resulting file is nowhere to be found. I also don't see Sphinx being run if I change the rst files.

SimonBiggs · 2019-07-05T09:36:19Z

I can not get the dev server to work unfortunately.
If I run yarn build I get a freshly compiled theme.js in sphinx_rtd_theme/static/js.
If I run yarn start I see that theme.js is recompiled every time I make a change, but the resulting file is nowhere to be found. I also don't see Sphinx being run if I change the rst files.

Happy to have this pull request closed. I thought it was close to parity, but obviously it is still missing too much.

agjohnson · 2019-07-17T23:58:59Z

@SimonBiggs I'd love to see this PR merged, let me know how I can help. Otherwise, I'll probably pull this down and poke it more.

I don't have really strong opinions about the stack changes, but webpack replaces a lot of fragile tooling and has shown to be a huge improvement over our gulp build system, and looks like a similar improvement over grunt.

I don't have strong opinions on yarn, but the core team doesn't do much JS. Having less JS dependencies is a strong benefit for us, and the reason we're targeting webpack for RTD. If npm can do everything that yarn is doing here, I'm +1 on skipping yarn.

SimonBiggs · 2019-07-18T02:34:37Z

@agjohnson I'm more than happy to have yarn replaced for npm. There are certainly some benefits, but not enough to force people to step away from what they're used to.

I think the best bet would be for you to jump in and poke it, feel free to push to this branch. I can help out if you have questions about decisions I made.

* Fix issues on paths in build and dev files * Skip yarn to reduce tooling requirements * Minify CSS assets in production * Reduce imports on fonts, hardcode font pieces more. There are problems with the font family names. Roboto mixin brings in everything as `Roboto-Slab-Bold`, which won't match local font name of 'Roboto Slab' * Fix the development instance more: * Add file watching on reST files * Execute build docs command after webpack build Things I'm still not sure about: - [ ] The actual JS output, I haven't vetted this yet - [ ] If woff2 and woff are enough for fonts. The NPM packages are missing other formats

agjohnson · 2019-07-18T05:49:33Z

@SimonBiggs Sounds good. I opened up a PR with what I've changed to get going: #781

Some notes about my attempt:

node-sass fails without python2 -- more specifically node-gyp fails, it is not yet compatible with python3. RTD has switched core to using python3 however. I guess we need to note that python 2.7 is required to run this until node-gyp finally goes python3? This is suboptimal
Core team isn't super flexible on node versions. Hopefully webpack isn't as prone to being picky about versions, but our current Gulp system needs node 8.x to run for some nondeterministic reason -- so that's what core team is stuck with atm :( I use nodenv/pyenv/etc for multiple version support, but I don't know what the rest of the team uses.

SimonBiggs · 2019-07-18T06:54:27Z

node-sass fails without python2 -- more specifically node-gyp fails, it is not yet compatible with python3. RTD has switched core to using python3 however. I guess we need to note that python 2.7 is required to run this until node-gyp finally goes python3? This is suboptimal

node-gyp v4.0.0 already works with Python 3:

https://github.com/nodejs/node-gyp/blob/master/CHANGELOG.md#v400-2019-04-24

Upgrading to version 4.0.0 of node-gyp should fix that issue I would imagine.

Core team isn't super flexible on node versions. Hopefully webpack isn't as prone to being picky about versions, but our current Gulp system needs node 8.x to run for some nondeterministic reason -- so that's what core team is stuck with atm :( I use nodenv/pyenv/etc for multiple version support, but I don't know what the rest of the team uses.

Not sure about that unfortunately.

jessetan · 2019-07-18T08:28:11Z

Re. Node, version 12.1.0 worked fine after bumping the version of bourbon-neat

agjohnson · 2019-07-18T23:07:39Z

node-gyp v4.0.0 already works with Python 3

Ah! I looked into this a bit more. It's actually bourbon-neat bringing in an old node-sass, tied to an old node-gyp. Updating to the latest bourbon-neat fixes the python 2/3 issue 👍

But now nothing builds.

This is wyrm showing it's age though, all the errors are from changes to Neat. I'm not sure the best thing to do here.

Node, version 12.1.0 worked fine after bumping the version of bourbon-neat

So yeah, ultimately a new bourbon-neat fixes a couple of issues then.

Our options, in no particular order of awfulness:

Try to get fixes into wyrm. I don't think snide has bandwidth for this.
Fork wyrm, apply fixes. Frankly, wyrm doesn't get much use besides us, we might as well maintain it in some fashion. We might be able to see about reassignment of the repo.
Vendor wyrm until we figure things out
Use another framework -- big 👎 from me here, I'd rather put this type of effort it a separate theme

Any thoughts? @jessetan and @Blendify specifically

I vote we do a light fork of wyrm into the readthedocs org, fix the Neat issues, install our fork from github here, and worry about the question of maintainership later.

For this PR, are we comfortable merging this with Python 2 and Node 10 support? We'd hopefully pretty quickly figure out the issues with Wyrm and support Python 3 and Node 10+. We can block otherwise, but I'm fine accepting Python 2 + Node 10 for a short while.

@SimonBiggs if you haven't already, feel free to either merge my branch in here, or give us access to your branch via maintainer permissions:
https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork

@jessetan it might be good to have you poke the PR i added some fixes on, and see if it solves the issues you hit (minus the obvious issues with Python/Node of course)

SimonBiggs · 2019-07-18T23:13:30Z

But now nothing builds.

This is wyrm showing it's age though, all the errors are from changes to Neat. I'm not sure the best thing to do here.

My yarn lock file has all the exact version in it that successfully build with this configuration...

SimonBiggs · 2019-07-18T23:16:04Z

@SimonBiggs if you haven't already, feel free to either merge my branch in here, or give us access to your branch via maintainer permissions:
help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork

Already have:

However I believe you set your pull request to target the branch on this repo:

So I don't have merge permissions here.

SimonBiggs · 2019-07-18T23:17:19Z

Try to get fixes into wyrm. I don't think snide has bandwidth for this.

Fork wyrm, apply fixes. Frankly, wyrm doesn't get much use besides us, we might as well maintain it in some fashion. We might be able to see about reassignment of the repo.

Vendor wyrm until we figure things out

Use another framework -- big 👎 from me here, I'd rather put this type of effort it a separate theme

None of these are needed :) The biggest thing I added here I believe is probably my "yarn lock file". Which was hours of dependency iteration finding the right combo that requires no changes from wyrm etc.

agjohnson · 2019-07-19T01:02:34Z

My yarn lock file has all the exact version in it that successfully build with this configuration...

I tried yarn initially and hit installation issues with the requirements. Specifically, bourbon-neat does still pin an old version of node-sass, which brings in node-gyp 3.8, and that is what fails with python 3. This doesn't seem to be a yarn vs npm thing, using python 2 I can install and build with both npm and yarn.

None of these are needed :) The biggest thing I added here I believe is probably my "yarn lock file". Which was hours of dependency iteration finding the right combo that requires no changes from wyrm etc.

This PR does build assets correctly, however just not with python 3 or node 12 (apparently, i didn't try yet). I'm describing fixing these issues later though, which requires a new version of Neat, which will require changes to Wyrm.

SimonBiggs · 2019-07-19T01:23:33Z

My yarn lock file has all the exact version in it that successfully build with this configuration...

I tried yarn initially and hit installation issues with the requirements. Specifically, bourbon-neat does still pin an old version of node-sass, which brings in node-gyp 3.8, and that is what fails with python 3. This doesn't seem to be a yarn vs npm thing, using python 2 I can install and build with both npm and yarn.

None of these are needed :) The biggest thing I added here I believe is probably my "yarn lock file". Which was hours of dependency iteration finding the right combo that requires no changes from wyrm etc.

This PR does build assets correctly, however just not with python 3 or node 12 (apparently, i didn't try yet). I'm describing fixing these issues later though, which requires a new version of Neat, which will require changes to Wyrm.

kk :)

agjohnson · 2019-07-19T04:49:45Z

I took a quick swing at upgrading Neat and it wasn't bad. I have the theme building with the upgrade (not well, but it builds). The rest of the Wyrm tool chain is more broken and not documented -- kss, node-sass, etc need more attention than the Neat upgrade.

agjohnson/wyrm#1

So I think it's doable without a lot of effort, but I've changed my mind on maintainership. I don't want to be responsible for maintaining this. I think a vendor fork is the best path forward, allowing us to continue with this PR.

I have some additions to my PR, I'll merge into this branch after.

jessetan · 2019-07-19T11:47:37Z

@agjohnson which version of bourbon-neat did you use? I've found that version 1.9 works with Node 12 and python 3. It is the last one in the 1.x branch. From version 2.x the incompatible changes start.

agjohnson · 2019-07-19T16:54:11Z

@jessetan yes! 💯 that does it for me, on a fresh install even.

I skipped even testing 1.9, assuming the whole 1.x series has the bad deps.

So that, at least for now, puts us in maintainable place. We probably still need to fork Wyrm eventually, but we can kick that can way down the road.

agjohnson · 2019-07-19T17:14:35Z

Okay. Everything is merged here. The last change I'd like some feedback on is:
cb87d8f#diff-b9cfc7f2cdf78a7f4b91a753d10865a2

It's proper to make all of these dev dependencies, no? We don't require any of these to run sphinx_rtd_theme, just to work on the theme.

I think I'm 👍 on merging after some docs are updated. I'd like to merge translation docs before reworking docs here though.

Code review on this PR is not super helpful, it's probably most helpful to check out the branch and try:

npm install
npm run build
npm run start

npm install should pass, npm run build should create css/js assets without changes to the repo, npm run start should build these assets, and also build docs on any changes, live reloading on port 1919.

SimonBiggs · 2019-07-19T21:10:56Z

Okay. Everything is merged here. The last change I'd like some feedback on is:
cb87d8f#diff-b9cfc7f2cdf78a7f4b91a753d10865a2

It's proper to make all of these dev dependencies, no? We don't require any of these to run sphinx_rtd_theme, just to work on the theme.

I believe any dependency which is for something that strictly helps with the development process (like linting for example) they belong in dev dependencies. Anything that is required to make "npm build" work, they belong in dependencies.

There is a flag that can be passed to npm install which only installs the dependencies and not dev dependencies. That can be used in the build step of a continuous deployment pipeline to only download what is necessary for the build.

jessetan · 2019-07-20T14:47:17Z

I think all our dependencies are dev dependencies, since they are used during development. The theme is never installed using npm with dependencies.

jessetan · 2019-07-20T15:12:22Z

All npm commands work as expected.

Three remarks:

If there is no built documentation in docs/build (i.e. during the first run), npm run start will show "Cannot GET /" in the browser and does not reload.
npm run start -> npm run dev?
I'm missing a npm run clean or similar command. Might not be needed but it has been useful in the past.

agjohnson · 2019-07-22T17:00:58Z

Also, npm says:

"dependencies": Packages required by your application in production.

Which would be no packages. I'll leave these dependencies in dev dependencies for now.

If there is no built documentation in docs/build (i.e. during the first run), npm run start will show Cannot GET /" in the browser and does not reload.

Good point. I'll see if there is a way to make the dev server avoid this.

npm run start -> npm run dev?

👍 I actually made this change, but reverted it. npm run start is normally the standard, but I agree npm run dev makes more sense

I'm missing a npm run clean or similar command. Might not be needed but it has been useful in the past.

That should be easy enough to add and seems like it would be useful.

changes have been addressed

agjohnson · 2019-07-25T19:47:55Z

Tests are all breaking because of a change introduced I believe in #405, where setup.py is importing the sphinx_rtd_theme package to get the __version__. This doesn't work of course, especially because we import Sphinx in the package. Instead of blocking more on this PR, I'll address with a separate PR right after.

Edit: Bah, nevermind. I have doc updates due here anyways. I'll do it the right way.

docs/contributing.rst

davidfischer

npm install
npm run build
npm run start

This ran fine for me although I have the following notes:

npm run build did show changes in the repo (sphinx_rtd_theme/static/css/theme.css) after running.
npm run start ran successfully for me although I had a few issues with making changes/rebuilding/live-reloading. It would sometimes refresh to a directory listing page when sphinx is rebuilding. Likewise, I had the same "Cannot GET /" issue as @jessetan. Both of these were fairly minor, IMO.
I tested making a change while running npm run start and the change was visible after reloading.

davidfischer · 2019-07-25T21:22:06Z

setup.py

@@ -55,7 +53,7 @@ def run(self):

 setup(
    name='sphinx_rtd_theme',
-    version=__version__,
+    version='0.4.3.dev0',


I see that you're addressing this in another PR. You can also consider reading the version from package.json. Unfortunately, that file can't also read the version from the Python theme module (and the theme module can't read from package.json). I've seen a couple approaches here from reading from package.json to having a separate file with just the version (eg. sphinx_rtd_theme/version.py) to parsing the text of sphinx_rtd_theme/__init__.py instead of importing it. I don't have a particularly strong opinion although any of them seem preferable to hardcoding it.

docs/contributing.rst

davidfischer · 2019-07-25T21:49:49Z

Overall, this is a huge improvement over what was there before, IMO. Thanks a lot for the contribution and persistence @SimonBiggs.

agjohnson · 2019-07-25T22:24:16Z

Merging! 🚀

Thanks everyone! And thanks again @SimonBiggs for getting this rolling!

SimonBiggs · 2019-07-25T22:24:57Z

Overall, this is a huge improvement over what was there before, IMO. Thanks a lot for the contribution and persistence @SimonBiggs.

My pleasure :) thank you for the improvements on it, and for the maintenance of this theme :)

successful build with webpack

beca6ea

SimonBiggs commented Jul 2, 2019

View reviewed changes

src/sass/_theme_font_local.sass Outdated Show resolved Hide resolved

commit the built files

0ab7645

SimonBiggs mentioned this pull request Jul 2, 2019

Working hot-reload converted over to webpack with all dependencies managed by yarn #757

Closed

jessetan previously requested changes Jul 4, 2019

View reviewed changes

jessetan reviewed Jul 4, 2019

View reviewed changes

package.json Outdated Show resolved Hide resolved

agjohnson mentioned this pull request Jul 18, 2019

A few additions for migration to webpack #781

Closed

2 tasks

Change port and do make clean build

fb934e0

Pin Neat v1.9 to avoid node-gyp 3.8

af7d610

agjohnson added 2 commits July 19, 2019 10:57

Merge branch 'agj/more-webpack' into swap-to-webpack

c5d01ad

Drop yarn lock, move deps to devdeps in package.json

cb87d8f

SimonBiggs mentioned this pull request Jul 19, 2019

Merge in the webpack docs improvements pymedphys/pymedphys#428

Closed

jessetan mentioned this pull request Jul 25, 2019

Plotly plots rendered by nbsphinx not showing in sphinx_rtd_theme #788

Closed

agjohnson mentioned this pull request Jul 25, 2019

Update dev dependencies #790

Merged

Merge branch 'master' into swap-to-webpack

9a8699b

Final doc updates and fix tests

472b3ae

agjohnson mentioned this pull request Jul 25, 2019

Put all build commands in setup.py command classes #797

Closed

SimonBiggs commented Jul 25, 2019

View reviewed changes

docs/contributing.rst Outdated Show resolved Hide resolved

docs/contributing.rst Show resolved Hide resolved

Review fixes on typo and missing change to package.json

94d5743

davidfischer approved these changes Jul 25, 2019

View reviewed changes

Forgot a changelog entry

a591817

agjohnson merged commit 8fbf2c0 into readthedocs:master Jul 25, 2019

agjohnson mentioned this pull request Jan 17, 2022

CI is failing #1275

Closed

This was referenced Aug 17, 2022

Python 3 support for node-js build stack #1311

Closed

Build assets in Docker container + use same container setup in CI #1312

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Swap to using webpack #771

Swap to using webpack #771

SimonBiggs commented Jul 2, 2019

jessetan left a comment

SimonBiggs commented Jul 4, 2019 •

edited

Loading

jessetan commented Jul 5, 2019

SimonBiggs commented Jul 5, 2019

agjohnson commented Jul 17, 2019

SimonBiggs commented Jul 18, 2019

agjohnson commented Jul 18, 2019

SimonBiggs commented Jul 18, 2019 •

edited

Loading

jessetan commented Jul 18, 2019

agjohnson commented Jul 18, 2019

SimonBiggs commented Jul 18, 2019

SimonBiggs commented Jul 18, 2019

SimonBiggs commented Jul 18, 2019

agjohnson commented Jul 19, 2019

SimonBiggs commented Jul 19, 2019

agjohnson commented Jul 19, 2019

jessetan commented Jul 19, 2019

agjohnson commented Jul 19, 2019

agjohnson commented Jul 19, 2019

SimonBiggs commented Jul 19, 2019

jessetan commented Jul 20, 2019

jessetan commented Jul 20, 2019

agjohnson commented Jul 22, 2019 •

edited

Loading

agjohnson commented Jul 25, 2019 •

edited

Loading

davidfischer left a comment

davidfischer Jul 25, 2019

davidfischer commented Jul 25, 2019

agjohnson commented Jul 25, 2019

SimonBiggs commented Jul 25, 2019

Swap to using webpack #771

Swap to using webpack #771

Conversation

SimonBiggs commented Jul 2, 2019

jessetan left a comment

Choose a reason for hiding this comment

SimonBiggs commented Jul 4, 2019 • edited Loading

jessetan commented Jul 5, 2019

SimonBiggs commented Jul 5, 2019

agjohnson commented Jul 17, 2019

SimonBiggs commented Jul 18, 2019

agjohnson commented Jul 18, 2019

SimonBiggs commented Jul 18, 2019 • edited Loading

jessetan commented Jul 18, 2019

agjohnson commented Jul 18, 2019

SimonBiggs commented Jul 18, 2019

SimonBiggs commented Jul 18, 2019

SimonBiggs commented Jul 18, 2019

agjohnson commented Jul 19, 2019

SimonBiggs commented Jul 19, 2019

agjohnson commented Jul 19, 2019

jessetan commented Jul 19, 2019

agjohnson commented Jul 19, 2019

agjohnson commented Jul 19, 2019

SimonBiggs commented Jul 19, 2019

jessetan commented Jul 20, 2019

jessetan commented Jul 20, 2019

agjohnson commented Jul 22, 2019 • edited Loading

agjohnson commented Jul 25, 2019 • edited Loading

davidfischer left a comment

Choose a reason for hiding this comment

davidfischer Jul 25, 2019

Choose a reason for hiding this comment

davidfischer commented Jul 25, 2019

agjohnson commented Jul 25, 2019

SimonBiggs commented Jul 25, 2019

SimonBiggs commented Jul 4, 2019 •

edited

Loading

SimonBiggs commented Jul 18, 2019 •

edited

Loading

agjohnson commented Jul 22, 2019 •

edited

Loading

agjohnson commented Jul 25, 2019 •

edited

Loading