Skip to content

Commit

Permalink
build: include built-in XBlock JS directly rather than copying it (#3…
Browse files Browse the repository at this point in the history 
…2480)

As part of the static asset build, JS modules for most built-in XBlocks were
unnecessarily copied from the original locations (under xmodule/js and
common/static/js) to a git-ignored location (under common/static/xmodule), and
then included into the Webpack builld via
common/static/xmodule/webpack.xmodule.config.js.

With this commit, we stop copying the JS modules. Instead, we have
common/static/xmodule/webpack.xmodule.config.js just reference the original
source under xmodule/js and common/static/js.

This lets us us radically simplify the xmodule/static_content.py build script.
It also sets the stage for the next change, in which we will check
webpack.xmodule.config.js into the repository, and delete
xmodule/static_content.py entirely.

common/static/xmodule/webpack.xmodule.config.js before:

    module.exports = {
        "entry": {
            "AboutBlockDisplay": [
                "./common/static/xmodule/modules/js/000-b82f6c436159f6bc7ca2513e29e82503.js",
                "./common/static/xmodule/modules/js/001-3ed86006526f75d6c844739193a84c11.js",
                "./common/static/xmodule/modules/js/002-3918b2d4f383c04fed8227cc9f523d6e.js",
                "./common/static/xmodule/modules/js/003-b3206f2283964743c4772b9d72c67d64.js",
                "./common/static/xmodule/modules/js/004-274b8109ca3426c2a6fde9ec2c56e969.js",
                "./common/static/xmodule/modules/js/005-26caba6f71877f63a7dd4f6796109bf6.js"
            ],
            "AboutBlockEditor": [
                "./common/static/xmodule/descriptors/js/000-b82f6c436159f6bc7ca2513e29e82503.js",
                "./common/static/xmodule/descriptors/js/001-19c4723cecaa5a5a46b8566b3544e732.js"
            ],
            // etc
        }
    };

common/static/xmodule/webpack.xmodule.config.js after:

    module.exports = {
        "entry": {
            "AboutBlockDisplay": [
                "./xmodule/js/src/xmodule.js",
                "./xmodule/js/src/html/display.js",
                "./xmodule/js/src/javascript_loader.js",
                "./xmodule/js/src/collapsible.js",
                "./xmodule/js/src/html/imageModal.js",
                "./xmodule/js/common_static/js/vendor/draggabilly.js"
            ],
            "AboutBlockEditor": [
                "./xmodule/js/src/xmodule.js",
                "./xmodule/js/src/html/edit.js"
            ],
            // etc
        }
    };

Part of: #32481
  • Loading branch information
@kdmccormick
kdmccormick authored Jul 26, 2023
1 parent 51079e5 commit 9d4163d
Show file tree
Hide file tree
Showing 6 changed files with 174 additions and 135 deletions.
2 changes: 0 additions & 2 deletions conf/locale/config.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,6 @@ dummy_locales:

# Directories we don't search for strings.
ignore_dirs:
- common/static/xmodule/modules
- common/static/xmodule/descriptors
# Directories with no user-facing code.
- '*/migrations'
- '*/envs'
Expand Down
4 changes: 1 addition & 3 deletions webpack-config/file-lists.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,7 @@ module.exports = {
path.resolve(__dirname, '../common/static/common/js/components/views/paging_footer.js'),
path.resolve(__dirname, '../cms/static/js/views/paging.js'),
path.resolve(__dirname, '../common/static/common/js/components/utils/view_utils.js'),
/descriptors\/js/,
/modules\/js/,
/xmodule\/js\/src\//,
/xmodule\/js\/src/,
path.resolve(__dirname, '../openedx/features/course_bookmarks/static/course_bookmarks/js/views/bookmark_button.js')
],

Expand Down
118 changes: 113 additions & 5 deletions webpack.common.config.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,7 @@ var xmoduleJS = require('./common/static/xmodule/webpack.xmodule.config.js');

var filesWithRequireJSBlocks = [
path.resolve(__dirname, 'common/static/common/js/components/utils/view_utils.js'),
/descriptors\/js/,
/modules\/js/,
/xmodule\/js\/src\//
/xmodule\/js\/src/
];

var defineHeader = /\(function ?\(((define|require|requirejs|\$)(, )?)+\) ?\{/;
Expand Down Expand Up @@ -311,14 +309,124 @@ module.exports = Merge.smart({
test: /xblock\/runtime.v1/,
loader: 'exports-loader?window.XBlock!imports-loader?XBlock=xblock/core,this=>window'
},
/*******************************************************************************************************
/* BUILT-IN XBLOCK ASSETS WITH GLOBAL DEFINITIONS:
*
* The monstrous list of globally-namespace modules below is the result of a JS build refactoring.
* Originally, all of these modules were copied to common/static/xmodule/js/[module|descriptors]/, and
* this file simply contained the lines:
*
* {
* test: /descriptors\/js/,
* loader: 'imports-loader?this=>window'
* },
* {
* test: /modules\/js/,
* loader: 'imports-loader?this=>window'
* },
*
* We removed that asset copying because it added complexity to the build, but as a result, in order to
* preserve exact parity with the preexisting global namespace, we had to enumerate all formely-copied
* modules here. It is very likely that many of these modules do not need to be in this list. Future
* refactorings are welcome to try to prune the list down to the minimal set of modules. As far as
* we know, the only modules that absolutely need to be added to the global namespace are those
* which define module types, for example "Problem" (in xmodule/js/src/capa/display.js).
*/
{
test: /descriptors\/js/,
test: /xmodule\/assets\/word_cloud\/src\/js\/word_cloud.js/,
loader: 'imports-loader?this=>window'
},
{
test: /modules\/js/,
test: /xmodule\/js\/common_static\/js\/vendor\/draggabilly.js/,
loader: 'imports-loader?this=>window'
},
{
test: /xmodule\/js\/src\/annotatable\/display.js/,
loader: 'imports-loader?this=>window'
},
{
test: /xmodule\/js\/src\/capa\/display.js/,
loader: 'imports-loader?this=>window'
},
{
test: /xmodule\/js\/src\/capa\/imageinput.js/,
loader: 'imports-loader?this=>window'
},
{
test: /xmodule\/js\/src\/capa\/schematic.js/,
loader: 'imports-loader?this=>window'
},
{
test: /xmodule\/js\/src\/collapsible.js/,
loader: 'imports-loader?this=>window'
},
{
test: /xmodule\/js\/src\/conditional\/display.js/,
loader: 'imports-loader?this=>window'
},
{
test: /xmodule\/js\/src\/html\/display.js/,
loader: 'imports-loader?this=>window'
},
{
test: /xmodule\/js\/src\/html\/edit.js/,
loader: 'imports-loader?this=>window'
},
{
test: /xmodule\/js\/src\/html\/imageModal.js/,
loader: 'imports-loader?this=>window'
},
{
test: /xmodule\/js\/src\/javascript_loader.js/,
loader: 'imports-loader?this=>window'
},
{
test: /xmodule\/js\/src\/lti\/lti.js/,
loader: 'imports-loader?this=>window'
},
{
test: /xmodule\/js\/src\/poll\/poll.js/,
loader: 'imports-loader?this=>window'
},
{
test: /xmodule\/js\/src\/poll\/poll_main.js/,
loader: 'imports-loader?this=>window'
},
{
test: /xmodule\/js\/src\/problem\/edit.js/,
loader: 'imports-loader?this=>window'
},
{
test: /xmodule\/js\/src\/raw\/edit\/metadata-only.js/,
loader: 'imports-loader?this=>window'
},
{
test: /xmodule\/js\/src\/raw\/edit\/xml.js/,
loader: 'imports-loader?this=>window'
},
{
test: /xmodule\/js\/src\/sequence\/display.js/,
loader: 'imports-loader?this=>window'
},
{
test: /xmodule\/js\/src\/sequence\/edit.js/,
loader: 'imports-loader?this=>window'
},
{
test: /xmodule\/js\/src\/tabs\/tabs-aggregator.js/,
loader: 'imports-loader?this=>window'
},
{
test: /xmodule\/js\/src\/vertical\/edit.js/,
loader: 'imports-loader?this=>window'
},
{
test: /xmodule\/js\/src\/video\/10_main.js/,
loader: 'imports-loader?this=>window'
},
/*
* END BUILT-IN XBLOCK ASSETS WITH GLOBAL DEFINITIONS
******************************************************************************************************/
{
test: /codemirror/,
loader: 'exports-loader?window.CodeMirror'
Expand Down
4 changes: 1 addition & 3 deletions xmodule/assets/README.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,7 @@ Currently, edx-platform XBlock JS is defined both here in `xmodule/assets`_ and

* For many older blocks, their JS is:

* copied to ``common/static/xmodule`` by `static_content.py`_ (aka ``xmodule_assets``),
* then bundled using a generated Webpack config at ``common/static/xmodule/webpack.xmodule.config.js``,
* bundled using a generated Webpack config at ``common/static/xmodule/webpack.xmodule.config.js``,
* which is included into `webpack.common.config.js`_,
* allowing it to be included into XBlock fragments using ``add_webpack_js_to_fragment`` from `builtin_assets.py`_.

Expand All @@ -77,7 +76,6 @@ Currently, edx-platform XBlock JS is defined both here in `xmodule/assets`_ and

As part of an `active build refactoring`_:

* We update the older builtin XBlocks to reference their JS directly rather than using copies of it.
* We will move ``webpack.xmodule.config.js`` here instead of generating it.
* We will consolidate all edx-platform XBlock JS here in `xmodule/assets`_.
* We will delete the ``xmodule_assets`` script.
Expand Down
Loading

0 comments on commit 9d4163d

Please sign in to comment.