Skip to content
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 watch (experimental) #32866

Merged
merged 1 commit into from
Jul 27, 2023

Conversation

kdmccormick
Copy link
Member

@kdmccormick kdmccormick commented Jul 27, 2023

Description and supporting info

Implements the npm run watch section of the assets ADR [1], plus some modifications since I decided to switch from pywatchman to watchdog (see ADR changes for justification). This will replace paver watch_assets (edx-platform) and openedx-assets watch-themes (Tutor).

There are no breaking changes in this PR. It does not touch the existing asset-watching tooling.

Specifically, this PR adds three experimental commands:

  • npm run watch-sass : Watch for Sass changes with watchdog.
  • npm run watch-webpack : Invoke Webpack-watch for JS changes.
  • npm run watch : Invoke both watch-sass and watch-webpack simultaneously.

These commands are only intended to work in development mode.
They have been tested both on bare-metal edx-platform and through tutor dev run on on Linux.

Before removing the "experimental" label, we need to:

  • Test through Devstack on Linux.
  • Test through Devstack and tutor dev run on macOS.
  • Test on bare-metal macOS. Might not work, which is OK, but we should document that.
  • Document the commands in edx-platform's README.
  • Confirm that this not only works through tutor dev run, but also as a suitable replacement in the watchthemes service that Tutor runs automatically as part of tutor dev start. Tweak if necessary.

References:

  1. https://github.com/openedx/edx-platform/blob/master/docs/decisions/0017-reimplement-asset-processing.rst

Part of: #31612

Testing

Either directly on your host or in a edx-platform-mounted tutor dev run [lms|cms] bash shell:

# Install requirements, if you don't have them already:
npm clean-install
pip install -r requirements/edx/development.txt

# Choose one of these:
npm run watch-webpack  # Only watch Webpack-managed assets (mostly JS).
npm run watch-sass     # Watch default Sass.
npm run watch          # Watch both at once (note: the output is a little harder to follow).
EDX_PLATFORM_THEME_DIRS=./themes npm run watch-sass # Watch default Sass, and Sass in the ./themes/ dir.
EDX_PLATFORM_THEME_DIRS=./themes npm run watch      # Same as a bove, but also watch Webpack-managed assets.

This will start the watcher. Now, in another shell, try making some changes in edx-platform:

 # UNWATCHED CHANGE: Nothing should recompile.
touch nope.scss
touch nope.js
rm nope.scss nope.js

# JS CHANGE: Webpack should recompile, if watched.
touch xmodule/js/src/capa/display.js

# XMODULE SASS CHANGE: All default Sass should recompile, if watched. Any watched themes should also recompile.
touch xmodule/assets/ProblemBlockDisplay.scss

# LMS SASS CHANGE: All default Sass should recompile, if watched. Any watched themes should also recompile.
touch lms/static/sass/lms-course.scss 

# CMS SASS CHANGE: CMS Sass should recompile, if watched. CMS Sass for watched themes should also recompile.
touch cms/static/sass/_build.scss

# UNWATCHED LMS CHANGE: Nothing should recompile.
touch lms/static/sass/this-is-not-sass.rs

# CERTS SASS CHANGE: Only default Sass should recompile, if watched. No watched themes should recompile.
touch lms/static/certificates/sass/main-rtl.scss

# THEME LMS CHANGE: Both LMS & CMS for Red Theme Sass should recompile, if watched.
touch themes/red-theme/lms/static/sass/partials/lms/theme/_variables.scss

# THEME CMS CHANGE: Only CMS Sass for Red Theme should recompile, if watched.
touch themes/red-theme/cms/static/sass/partials/cms/theme/_variables.scss

@kdmccormick kdmccormick force-pushed the kdmccormick/npm-run-watch branch 2 times, most recently from 2371200 to 5613824 Compare July 27, 2023 15:36
@kdmccormick kdmccormick marked this pull request as ready for review July 27, 2023 15:38
Copy link
Contributor

@feanil feanil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good, one suggestion I have is, to run the SCSS compile commands once before adding the relevant watcher. This would more closely matches the behavior I was expecting which is that running the watcher would do an initial compile and then update if there are changes. This is also what the webpack watcher is doing.

Without it, you have to make a new change once the watcher is running, not a hard thing to do but different behavior from the other watcher.

I don't think that blocks merging this change.

Implements the `npm run watch` section of the assets ADR [1], plus some
modifications since I decided to switch from pywatchman to watchdog (see
ADR changes for justification). This will replace `paver watch_assets`
(edx-platform) and `openedx-assets watch-themes` (Tutor).

Specifically, this PR adds three experimental commands:

* `npm run watch-sass` : Watch for Sass changes with watchdog.
* `npm run watch-webpack` : Invoke Webpack-watch for JS changes.
* `npm run watch` : Invoke both `watch-sass` and `watch-webpack` simultaneously.

These commands are only intended to work in development mode. They have
been tested both on bare-metal edx-platform and through `tutor dev run`
on on Linux.

Before removing the "experimental" label, we need to:

* Test through Devstack on Linux.
* Test through Devstack and `tutor dev run` on macOS.
* Test on bare-metal macOS. Might not work, which is OK, but we should
  document that.
* Document the commands in edx-platform's README.
* Confirm that this not only works through `tutor dev run`, but also as
  a suitable replacement in the `watchthemes` service that Tutor runs
  automatically as part of `tutor dev start`. Tweak if necessary.

References:

1. https://github.com/openedx/edx-platform/blob/master/docs/decisions/0017-reimplement-asset-processing.rst

Part of: openedx#31612 #
@kdmccormick
Copy link
Member Author

Good call @feanil. It was an easy change so I went ahead and made it.

@kdmccormick kdmccormick enabled auto-merge (squash) July 27, 2023 16:13
@kdmccormick kdmccormick merged commit 303bf5e into openedx:master Jul 27, 2023
41 checks passed
@kdmccormick kdmccormick deleted the kdmccormick/npm-run-watch branch July 27, 2023 16:33
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

Ali-Salman29 pushed a commit to edly-io/edx-platform that referenced this pull request Dec 4, 2023
Implements the `npm run watch` section of the assets ADR [1], plus some
modifications since I decided to switch from pywatchman to watchdog (see
ADR changes for justification). This will replace `paver watch_assets`
(edx-platform) and `openedx-assets watch-themes` (Tutor).

Specifically, this PR adds three experimental commands:

* `npm run watch-sass` : Watch for Sass changes with watchdog.
* `npm run watch-webpack` : Invoke Webpack-watch for JS changes.
* `npm run watch` : Invoke both `watch-sass` and `watch-webpack` simultaneously.

These commands are only intended to work in development mode. They have
been tested both on bare-metal edx-platform and through `tutor dev run`
on on Linux.

Before removing the "experimental" label, we need to:

* Test through Devstack on Linux.
* Test through Devstack and `tutor dev run` on macOS.
* Test on bare-metal macOS. Might not work, which is OK, but we should
  document that.
* Document the commands in edx-platform's README.
* Confirm that this not only works through `tutor dev run`, but also as
  a suitable replacement in the `watchthemes` service that Tutor runs
  automatically as part of `tutor dev start`. Tweak if necessary.

References:

1. https://github.com/openedx/edx-platform/blob/master/docs/decisions/0017-reimplement-asset-processing.rst

Part of: openedx#31612
@kdmccormick kdmccormick mentioned this pull request May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants