-
Notifications
You must be signed in to change notification settings - Fork 4.2k
build: npm run build (experimental)
#32823
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
build: npm run build (experimental)
#32823
Conversation
006ffe0 to
1175a73
Compare
kdmccormick
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notes for reviewers 👀
package.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd love to add comments here, but I can't since it's JSON. I do plan to document these in edx-platform's README, though, once they stabilize more.
In short:
npm run webpackis a super thin wrapper aroundwebpack.npm run compile-sasscalls a script (currently written in Python, maybe one day written in Bash) which compiles Sass into CSS.npm run buildis a combination of the above.- The
-devtargets are just simple variations of the base commands, for the convenience of developres. - Wherever it was possible, I've written these so that:
- You can override them with en vars, eg
WEBPACK_CONFIG_PATH=my-special-config.js npm run webpack - You can provide extra options:
npm run compile-sass -- --theme-dir=/my/cool/themes
- You can override them with en vars, eg
requirements/edx/paver.in
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Constraint is moved over to constraints.txt so that it can be shared by assets.txt.
scripts/compile_sass.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a new env var, and is mentioned in the ADR. Eventually, I will document it in the edx-platform README.
webpack.common.config.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goal of all the changes in this file is to provide some reasonable defaults for all env vars, so that webpack can be run on its own without having to supply a pile of environment variables.
We could have shoved all these defaults into the npm run webpack call in package.json, but that seemed worse than putting them here.
646d452 to
40578a4
Compare
feanil
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to run this after running make clean in edx-platform.
npm install --legacy-peer-deps
npm run build
I got the following error:
Error: Cannot find module './common/static/xmodule/webpack.xmodule.config.js'
Did a file not get checked in or did I miss a step somewhere?
feanil
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, doh, missed the bottom of the description. But yes that was it, it works now, I was able to get all the assets built locally without any issue.
40578a4 to
e1a88d9
Compare
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
|
2U Release Notice: This PR has been deployed to the edX production environment. |
This PR implements much of: https://github.com/openedx/edx-platform/blob/master/docs/decisions/0017-reimplement-asset-processing.rst
This is backwards-compatible.
paver update_assetsshould not be affected.The new command warns that it is "experimental" for a few reasons:
npm run buildwill fail in the webpack phase unless you first runxmodule_assets. This will be changed soon, so I did not feel that it would be worthwhile to add a separate specialnpm run xmodule-assetsstep.npm run build, its subcommands (npm run webpack[-dev],npm run compile-sass[-dev]), and the environemnt variables (STATIC_ROOT_LMS, etc).npm run watchis not yet implemented. The current asset-watching that edx-platform and Tutor provide both rely on Paver.Part of: #31604
Testing
There are a few different ways you could test this:
Directly from your host, preview the Sass help & output:
npm run compile-sass -- --help npm run compile-sass -- --dry --theme-dir=themes`Using Tutor Nightly dev:
Using Tutor Nightly prod:
If you're adventurous: directly from your host, try:
npm clean-install pip install -r requirements/edx/base.txt # (would be assets.txt if not for xmodule_assets command) xmodule_assets common/static/xmodule npm run build-dev