build: npm run webpack (WIP)

Dockerfile

@@ -117,6 +117,7 @@ RUN pip install -r requirements/edx/base.txt
 
 # Install node and node modules
 RUN nodeenv /edx/app/edxapp/nodeenv --node=16.14.0 --prebuilt
+COPY scripts/copy-node-modules.sh scripts
 RUN npm install -g [email protected]
 COPY package.json package.json
 COPY package-lock.json package-lock.json

docs/decisions/0017-reimplement-asset-processing.rst

@@ -131,17 +131,17 @@ The three top-level edx-platform asset processing actions are *build*, *collect*
      - ``scripts/build-assets.sh``
 
        A Bash script that contains all build stages, with subcommands available for running each stage separately. Its command-line interface inspired by Tutor's ``openedx-assets`` script. The script will be runnable on any POSIX system, including macOS and Ubuntu and it will linted for common shell scripting mistakes using `shellcheck <https://www.shellcheck.net>`_.
-     
+
    * - + **Build stage 1: Copy npm-installed assets** from node_modules to other folders in edx-platform. They are used by certain especially-old legacy LMS & CMS frontends that are not set up to work with npm directly.
 
      - ``paver update_assets --skip-collect``
 
        Implemented in Python within update_assets. There is no standalone command for it.
 
-     - ``scripts/build-assets.sh npm``
+     - ``npm install``
+
+       An NPM post-install hook will automatically call scripts/copy-node-modules.sh, a pure Bash reimplementation of the node_modules asset copying, whenever ``npm install`` is invoked.
 
-       Pure Bash reimplementation. See *Rejected Alternatives* for a note about this.
-
    * - + **Build stage 2: Copy XModule fragments** from the xmodule source tree over to input directories for Webpack and SCSS compilation. This is required for a hard-coded list of old XModule-style XBlocks. This is not required for new pure XBlocks, which include (or pip-install) their assets into edx-platform as ready-to-serve JS/CSS/etc fragments.
 
      - ``paver process_xmodule_assets``, or
@@ -150,25 +150,22 @@ The three top-level edx-platform asset processing actions are *build*, *collect*
 
        Equivalent paver task and console script, both pointing at to an application-level Python module. That module inspects attributes from legacy XModule-style XBlock classes in order to determine which static assets to copy and what to name them.
 
-     - ``scripts/build-assets.sh xmodule``
+     - (step no longer needed)
 
-       Initially, this command will just call out to the existing ``xmodule_assets`` command. Eventually, in order to make this step Python-free, we will need do either one or both of the following:
+       We will `remove the need for this step entirely <https://github.com/openedx/edx-platform/issues/31624>`_.
 
-       + `Reimplement this step in Bash <https://github.com/openedx/edx-platform/issues/31611>`_.
-       + `Remove the need for this step entirely <https://github.com/openedx/edx-platform/issues/31624>`_.
-
    * - + **Build stage 3: Run Webpack** in order to to shim, minify, otherwise process, and bundle JS modules. This requires a call to the npm-installed ``webpack`` binary.
 
      - ``paver webpack``
 
        Python wrapper around a call to webpack. Invokes the ``./manage.py [lms|cms] print_setting`` multiple times in order to determine Django settings, adding which can add 20+ seconds to the build.
 
-     - ``scripts/build-assets.sh webpack --static-root "$(./manage.py lms print_setting STATIC_ROOT)"``.
+     - ``npm run webpack`` or ``npm run webpack-dev``
 
-       Bash wrapper around a call to webpack. The script will accept parameters rather than looking up Django settings itself.
+       Simple shell script defined in package.json to invoke Webpack in prod or dev mode. The script will look for several environment variables, with a default defined for each one. See **Build Configuration** for details. The script will NOT invoke ``print_setting``; we leave to distributions the tasking of setting environment variables appropriately.
+
+       To continue using ``print_setting``, one could run: ``STATIC_ROOT_LMS="$(./manage.py lms print_setting STATIC_ROOT_LMS)" npm run webpack``
 
-       The print_setting command will still be available for distributions to use to extract ``STATIC_ROOT`` from Django settings, but it will only need to be run once. As described in **Build Configuration** below, unnecessary Django settings will be removed. Some distributions may not even need to look up ``STATIC_ROOT``; Tutor, for example, will probably render ``STATIC_ROOT`` directly into the environment variable ``OPENEDX_BUILD_ASSETS_OPTS`` variable, described in the **Build Configuration**.
-
    * - + **Build stage 4: Compile default SCSS** into CSS for legacy LMS/CMS frontends.
 
      - ``paver compile_sass``
@@ -180,7 +177,7 @@ The three top-level edx-platform asset processing actions are *build*, *collect*
      - ``scripts/build-assets.sh css``
 
        Bash reimplementation, calling ``node-sass`` and ``rtlcss``.
-   
+
        The initial implementation of build-assets.sh may use ``sassc``, a CLI provided by libsass, instead of node-sass. Then, ``sassc`` can be replaced by ``node-sass`` as part of a subsequent `edx-platform frontend framework upgrade effort <https://github.com/openedx/edx-platform/issues/31616>`_.
 
    * - + **Build stage 5: Compile themes' SCSS** into CSS for legacy LMS/CMS frontends. The default SCSS is used as a base, and theme-provided SCSS files are used as overrides. Themes are searched for from some number of operator-specified theme directories.
@@ -198,7 +195,7 @@ The three top-level edx-platform asset processing actions are *build*, *collect*
        The management command will remain available, but it will need to be updated to point at the Bash script, which will replace the paver task (see build stage 4 for details).
 
        The overall asset *build* action will use the Bash script; this means that list of theme directories will need to be provided as arguments, but it ensures that the build can remain Python-free.
-   
+
    * - **Collect** the built static assets from edx-platform to another location (the ``STATIC_ROOT``) so that they can be efficiently served *without* Django's webserver. This step, by nature, requires Python and Django in order to find and organize the assets, which may come from edx-platform itself or from its many installed Python and NPM packages. This is only needed for **production** environments, where it is usually desirable to serve assets with something efficient like NGINX.
 
      - ``paver update_assets``
@@ -210,7 +207,7 @@ The three top-level edx-platform asset processing actions are *build*, *collect*
      - ``./manage.py lms collectstatic --noinput && ./manage.py cms collectstatic --noinput``
 
        The standard Django interface will be used without a wrapper. The ignore patterns will be added to edx-platform's `staticfiles app configuration <https://docs.djangoproject.com/en/4.1/ref/contrib/staticfiles/#customizing-the-ignored-pattern-list>`_ so that they do not need to be supplied as part of the command.
-   
+
    * - **Watch** static assets for changes in the background. When a change occurs, rebuild them automatically, so that the Django webserver picks up the changes. This is only necessary in **development** environments. A few different sets of assets may be watched: XModule fragments, Webpack assets, default SCSS, and theme SCSS.
 
      - ``paver watch_assets``
@@ -264,6 +261,18 @@ Furthermore, to facilitate a Python-free build reimplementation, we will remove
          OPENEDX_BUILD_ASSETS_OPTS=\
          '--webpack-config path/to/webpack.my.config.js'
 
+   * - STATIC_ROOT_LMS
+
+     - TODO
+
+     - TODO
+
+   * - STATIC_ROOT_CMS
+
+     - TODO
+
+     - TODO
+
    * - JS_ENV_EXTRA_CONFIG
 
      - Global configuration object available to edx-platform JS modules. Defaults empty. Only known use is to add configuration and plugins for the TinyMCE editor.
@@ -300,13 +309,15 @@ Either way, the migration path is straightforward:
    * - ``openedx-assets build``
      - ``scripts/build-assets.sh``
    * - ``openedx-assets npm``
-     - ``scripts/build-assets.sh npm``
+     - ``scripts/copy-node-modules.sh  # (automatically invoked by 'npm install'!)
    * - ``openedx-assets xmodule``
-     - ``scripts/build-assets.sh xmodule``
+     - (no longer needed)
    * - ``openedx-assets common``
      - ``scripts/build-assets.sh css``
    * - ``openedx-assets themes``
      - ``scripts/build-assets.sh themes``
+   * - ``openedx-assets webpack [--env=dev]``
+     - ``npm run webpack[-dev]``
    * - ``openedx-assets collect``
      - ``./manage.py [lms|cms] collectstatic --noinput``
    * - ``openedx-assets watch-themes``
@@ -328,13 +339,6 @@ OpenCraft has also performed a discovery on a `modernized system for static asse
 Rejected Alternatives
 *********************
 
-Copy node_modules via npm post-install
-======================================
-
-It was noted that `npm supports lifecycle scripts <https://docs.npmjs.com/cli/v6/using-npm/scripts#pre--post-scripts>`_ in package.json, including ``postinstall``. We could use a post-install script to copy assets out of node_modules; this would occurr automatically after ``npm install``. Arguably, this would be more idiomatic than this ADR's proposal of ``scripts/build-assets.sh npm``.
-
-For now, we decided against this. While it seems like a good potential future improvement, we are currently unsure how it would interact with `moving node_modules out of edx-platform in Tutor <https://github.com/openedx/wg-developer-experience/issues/150>`_, which is a motivation behind this ADR. For example, if node_modules could be located anywhere on the image, then we are not sure how the post-install script could know its target directory without us hard-coding Tutor's directory structure into the script.
-
 Live with the problem
 ======================
 

lms/djangoapps/courseware/tests/test_discussion_xblock.py

@@ -218,7 +218,7 @@ def test_has_permission(self):
         """
         permission_canary = object()
         with mock.patch(
-            'lms.djangoapps.discussion.django_comment_client.permissions.has_permission',
+            'xmodule.discussion_block.has_permission',
             return_value=permission_canary,
         ) as has_perm:
             actual_permission = self.block.has_permission("test_permission")

package.json

@@ -2,6 +2,11 @@
   "name": "edx",
   "version": "0.1.0",
   "repository": "https://github.com/openedx/edx-platform",
+  "scripts": {
+    "postinstall": "scripts/copy-node-modules.sh",
+    "webpack": "NODE_ENV=${NODE_ENV:-production} \"$(npm bin)/webpack\" --progress --config=${WEBPACK_CONFIG_PATH:-webpack.prod.config.js}",
+    "webpack-dev": "NODE_ENV=development WEBPACK_CONFIG_PATH=webpack.dev.config.js npm run webpack"
+  },
   "dependencies": {
     "@babel/core": "7.19.0",
     "@babel/plugin-proposal-object-rest-spread": "^7.18.9",

pavelib/assets.py

@@ -46,39 +46,6 @@
     path('node_modules'),
 ]
 
-# A list of NPM installed libraries that should be copied into the common
-# static directory.
-# If string ends with '/' then all file in the directory will be copied.
-NPM_INSTALLED_LIBRARIES = [
-    'backbone.paginator/lib/backbone.paginator.js',
-    'backbone/backbone.js',
-    'bootstrap/dist/js/bootstrap.bundle.js',
-    'hls.js/dist/hls.js',
-    'jquery-migrate/dist/jquery-migrate.js',
-    'jquery.scrollto/jquery.scrollTo.js',
-    'jquery/dist/jquery.js',
-    'moment-timezone/builds/moment-timezone-with-data.js',
-    'moment/min/moment-with-locales.js',
-    'picturefill/dist/picturefill.js',
-    'requirejs/require.js',
-    'underscore.string/dist/underscore.string.js',
-    'underscore/underscore.js',
-    '@edx/studio-frontend/dist/',
-    'which-country/index.js'
-]
-
-# A list of NPM installed developer libraries that should be copied into the common
-# static directory only in development mode.
-NPM_INSTALLED_DEVELOPER_LIBRARIES = [
-    'sinon/pkg/sinon.js',
-    'squirejs/src/Squire.js',
-]
-
-# Directory to install static vendor files
-NPM_JS_VENDOR_DIRECTORY = path('common/static/common/js/vendor')
-NPM_CSS_VENDOR_DIRECTORY = path("common/static/common/css/vendor")
-NPM_CSS_DIRECTORY = path("common/static/common/css")
-
 # system specific lookup path additions, add sass dirs if one system depends on the sass files for other systems
 SASS_LOOKUP_DEPENDENCIES = {
     'cms': [path('lms') / 'static' / 'sass' / 'partials', ],
@@ -356,7 +323,6 @@ class SassWatcher(PatternMatchingEventHandler):
     """
     ignore_directories = True
     patterns = ['*.scss']
-    ignore_patterns = ['common/static/xmodule/*']
 
     def register(self, observer, directories):
         """
@@ -385,47 +351,6 @@ def on_any_event(self, event):
             traceback.print_exc()
 
 
-class XModuleSassWatcher(SassWatcher):
-    """
-    Watches for sass file changes
-    """
-    ignore_directories = True
-    ignore_patterns = []
-
-    @debounce()
-    def on_any_event(self, event):
-        print('\tCHANGED:', event.src_path)
-        try:
-            process_xmodule_assets()
-        except Exception:  # pylint: disable=broad-except
-            traceback.print_exc()
-
-
-class XModuleAssetsWatcher(PatternMatchingEventHandler):
-    """
-    Watches for css and js file changes
-    """
-    ignore_directories = True
-    patterns = ['*.css', '*.js']
-
-    def register(self, observer):
-        """
-        Register files with observer
-        """
-        observer.schedule(self, 'xmodule/', recursive=True)
-
-    @debounce()
-    def on_any_event(self, event):
-        print('\tCHANGED:', event.src_path)
-        try:
-            process_xmodule_assets()
-        except Exception:  # pylint: disable=broad-except
-            traceback.print_exc()
-
-        # To refresh the hash values of static xmodule content
-        restart_django_servers()
-
-
 @task
 @no_help
 @cmdopts([
@@ -644,73 +569,17 @@ def process_npm_assets():
     """
     Process vendor libraries installed via NPM.
     """
-    def copy_vendor_library(library, skip_if_missing=False):
-        """
-        Copies a vendor library to the shared vendor directory.
-        """
-        if library.startswith('node_modules/'):
-            library_path = library
-        else:
-            library_path = f'node_modules/{library}'
-
-        if library.endswith('.css') or library.endswith('.css.map'):
-            vendor_dir = NPM_CSS_VENDOR_DIRECTORY
-        else:
-            vendor_dir = NPM_JS_VENDOR_DIRECTORY
-        if os.path.exists(library_path):
-            sh('/bin/cp -rf {library_path} {vendor_dir}'.format(
-                library_path=library_path,
-                vendor_dir=vendor_dir,
-            ))
-        elif not skip_if_missing:
-            raise Exception(f'Missing vendor file {library_path}')
-
-    def copy_vendor_library_dir(library_dir, skip_if_missing=False):
-        """
-        Copies all vendor libraries in directory to the shared vendor directory.
-        """
-        library_dir_path = f'node_modules/{library_dir}'
-        print(f'Copying vendor library dir: {library_dir_path}')
-        if os.path.exists(library_dir_path):
-            for dirpath, _, filenames in os.walk(library_dir_path):
-                for filename in filenames:
-                    copy_vendor_library(os.path.join(dirpath, filename), skip_if_missing=skip_if_missing)
-
-    # Skip processing of the libraries if this is just a dry run
-    if tasks.environment.dry_run:
-        tasks.environment.info("install npm_assets")
-        return
-
-    # Ensure that the vendor directory exists
-    NPM_JS_VENDOR_DIRECTORY.mkdir_p()
-    NPM_CSS_DIRECTORY.mkdir_p()
-    NPM_CSS_VENDOR_DIRECTORY.mkdir_p()
-
-    # Copy each file to the vendor directory, overwriting any existing file.
-    print("Copying vendor files into static directory")
-    for library in NPM_INSTALLED_LIBRARIES:
-        if library.endswith('/'):
-            copy_vendor_library_dir(library)
-        else:
-            copy_vendor_library(library)
-
-    # Copy over each developer library too if they have been installed
-    print("Copying developer vendor files into static directory")
-    for library in NPM_INSTALLED_DEVELOPER_LIBRARIES:
-        copy_vendor_library(library, skip_if_missing=True)
+    sh('scripts/copy-node-modules.sh')
 
 
 @task
-@needs(
-    'pavelib.prereqs.install_python_prereqs',
-)
 @no_help
 def process_xmodule_assets():
     """
     Process XModule static assets.
     """
-    sh('xmodule_assets common/static/xmodule')
-    print("\t\tFinished processing xmodule assets.")
+    print("\t\tProcessing xmodule assets is no longer needed. This task is now a no-op.")
+    print("\t\tWhen paver is removed from edx-platform, this step will not replaced.")
 
 
 def restart_django_servers():
@@ -908,8 +777,6 @@ def watch_assets(options):
     observer = Observer(timeout=wait)
 
     SassWatcher().register(observer, sass_directories)
-    XModuleSassWatcher().register(observer, ['xmodule/'])
-    XModuleAssetsWatcher().register(observer)
 
     print("Starting asset watcher...")
     observer.start()
@@ -931,6 +798,7 @@ def watch_assets(options):
 @task
 @needs(
     'pavelib.prereqs.install_node_prereqs',
+    'pavelib.prereqs.install_python_prereqs',
 )
 @consume_args
 @timed
@@ -982,9 +850,6 @@ def update_assets(args):
     args = parser.parse_args(args)
     collect_log_args = {}
 
-    process_xmodule_assets()
-    process_npm_assets()
-
     # Build Webpack
     call_task('pavelib.assets.webpack', options={'settings': args.settings})
 

pavelib/js_test.py

@@ -26,7 +26,6 @@
 @needs(
     'pavelib.prereqs.install_node_prereqs',
     'pavelib.utils.test.utils.clean_reports_dir',
-    'pavelib.assets.process_xmodule_assets',
 )
 @cmdopts([
     ("suite=", "s", "Test suite to run"),

pavelib/paver_tests/test_servers.py

@@ -230,7 +230,6 @@ def verify_server_task(self, task_name, options):
             expected_asset_settings = "test_static_optimized"
         expected_collect_static = not is_fast and expected_settings != Env.DEVSTACK_SETTINGS
         if not is_fast:
-            expected_messages.append("xmodule_assets common/static/xmodule")
             expected_messages.append("install npm_assets")
             expected_messages.extend(
                 [c.format(settings=expected_asset_settings,
@@ -275,7 +274,6 @@ def verify_run_all_servers_task(self, options):
         expected_collect_static = not is_fast and expected_settings != Env.DEVSTACK_SETTINGS
         expected_messages = []
         if not is_fast:
-            expected_messages.append("xmodule_assets common/static/xmodule")
             expected_messages.append("install npm_assets")
             expected_messages.extend(
                 [c.format(settings=expected_asset_settings,

pavelib/utils/test/suites/js_suite.py

@@ -39,8 +39,6 @@ def __enter__(self):
         if self.mode == 'run' and not self.run_under_coverage:
             test_utils.clean_dir(self.report_dir)
 
-        assets.process_npm_assets()
-
     @property
     def _default_subsuites(self):
         """