-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
build: npm run watch
(experimental)
#32866
Conversation
2371200
to
5613824
Compare
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.
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 #
5613824
to
c6bc019
Compare
Good call @feanil. It was an easy change so I went ahead and made it. |
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. |
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. |
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
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 replacepaver watch_assets
(edx-platform) andopenedx-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 bothwatch-sass
andwatch-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:
tutor dev run
on macOS.tutor dev run
, but also as a suitable replacement in thewatchthemes
service that Tutor runs automatically as part oftutor dev start
. Tweak if necessary.References:
Part of: #31612
Testing
Either directly on your host or in a edx-platform-mounted
tutor dev run [lms|cms] bash
shell:This will start the watcher. Now, in another shell, try making some changes in edx-platform: