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

[2/3] build: include built-in XBlock JS directly rather than copying it #32480

Merged

Conversation

kdmccormick
Copy link
Member

@kdmccormick kdmccormick commented Jun 15, 2023

Part of a series of PRs:

The previous one is:

The next one is:

To review this PR, look at just the latest commit (the one matching the PR title).

Description and supporting info

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
    }
};

Testing

I recommend building assets with this branch and smoke-testing the built-in XBlocks.

You can build assets and start the platform with Tutor Nightly:

tutor local stop
tutor config save --set EDX_PLATFORM_REPOSITORY=https://github.com/kdmccormick/edx-platform
tutor config save --set EDX_PLATFORM_VERSION=kdmccormick/xmodule-js-no-copy
tutor images build openedx
tutor local start -d

or do it standalone (I haven't tried this):

paver update_assets
./manage.py lms runserver
./manage.py cms runserver

Go into the demo course and try out a few built-in XBlocks. Affected XBlocks include:

  • Sequentials aka subsections
  • any Text aka HTML
  • static tabs
  • Video
  • any Problem
  • Annotatable
  • Conditional
  • Randomized Content aka library_content_block
  • Poll (old)
  • LTI (old)
  • WordCloud

@kdmccormick kdmccormick changed the title build: commit XModule SCSS entrypoints of generating them build: include XModule JS directly rather than copying it Jun 15, 2023
@kdmccormick kdmccormick force-pushed the kdmccormick/xmodule-js-no-copy branch 6 times, most recently from fc8ae8c to 284ce1d Compare June 21, 2023 17:31
@kdmccormick kdmccormick force-pushed the kdmccormick/xmodule-js-no-copy branch from 284ce1d to a208e8e Compare July 7, 2023 17:52
@kdmccormick kdmccormick changed the title build: include XModule JS directly rather than copying it [2] build: include XModule JS directly rather than copying it Jul 7, 2023
@kdmccormick kdmccormick changed the title [2] build: include XModule JS directly rather than copying it [2/3] build: include XModule JS directly rather than copying it Jul 7, 2023
@kdmccormick kdmccormick force-pushed the kdmccormick/xmodule-js-no-copy branch from a208e8e to f9fda6a Compare July 7, 2023 18:23
@kdmccormick kdmccormick changed the title [2/3] build: include XModule JS directly rather than copying it [2/3] build: include built-in XBlock JS directly rather than copying it Jul 7, 2023
@kdmccormick kdmccormick force-pushed the kdmccormick/xmodule-js-no-copy branch 3 times, most recently from 66216d6 to 5391623 Compare July 7, 2023 18:57
@kdmccormick kdmccormick marked this pull request as ready for review July 10, 2023 19:42
@kdmccormick kdmccormick force-pushed the kdmccormick/xmodule-js-no-copy branch from 5391623 to 5c867ed Compare July 10, 2023 21:27
@kdmccormick kdmccormick marked this pull request as draft July 11, 2023 18:57
@kdmccormick kdmccormick force-pushed the kdmccormick/xmodule-js-no-copy branch 4 times, most recently from ead1eef to b99b7b1 Compare July 21, 2023 02:40
@kdmccormick kdmccormick force-pushed the kdmccormick/xmodule-js-no-copy branch from b99b7b1 to 8cb61df Compare July 24, 2023 21:14
@kdmccormick kdmccormick marked this pull request as ready for review July 25, 2023 14:45
@kdmccormick kdmccormick force-pushed the kdmccormick/xmodule-js-no-copy branch from 8cb61df to 2cdee1c Compare July 25, 2023 15:03
TODO: will copy in commit message from PR when squash merging

Part of: openedx#32481
@kdmccormick kdmccormick force-pushed the kdmccormick/xmodule-js-no-copy branch from 2cdee1c to e87b57d Compare July 26, 2023 13:06
@kdmccormick kdmccormick merged commit 9d4163d into openedx:master Jul 26, 2023
41 checks passed
@kdmccormick kdmccormick deleted the kdmccormick/xmodule-js-no-copy branch July 26, 2023 17:27
kdmccormick added a commit that referenced this pull request Jul 26, 2023
@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.

kdmccormick added a commit to kdmccormick/edx-platform that referenced this pull request Jul 27, 2023
The Webpack configuration file for built-in XBlock JS used to be
generated at build time and git-ignored. It lived at
common/static/xmodule/webpack.xmodule.config.js. It was generated
because the JS that it referred to was also generated at build-time, and
the filenames of those JS modules were not static.

Now that its contents have been made entirely static [1], there is no
reason we need to continue generating this Webpack configuration file.
So, we check it into edx-platform under the name
./webpack.builtinblocks.config.js. We choose to put it in the repo's
root directory because the paths contained in the config file are
relative to the repo's root.

This allows us to behead both the xmodule/static_content.py
(`xmodule_assets`) script andthe  `process_xmodule_assets` paver task, a
major step in removing the need for Python in the edx-platform asset
build [2]. It also allows us to delete the `HTMLSnippet` class and all
associated attributes, which were exclusively used by
xmodule/static_content.py..

We leave `xmodule_assets` and  `process_xmodule_assets` in as stubs for
now in order to avoid breaking external code (like Tutor) which calls
Paver; the entire pavelib/assets.py function will be eventually removed
soon anyway [3]. Further, to avoid extraneous refactoring, we keep one
method of `HTMLSnippet` around on a few of its former subclasses:
`get_html`. This method was originally part of the XModule framework;
now, it is left over on a few classes as a simple internal helper
method.

References:
1. openedx#32480
2. openedx#31800
3. openedx#31895

Part of: openedx#32481
kdmccormick added a commit that referenced this pull request Jul 27, 2023
…ts` (#32685)

The Webpack configuration file for built-in XBlock JS used to be
generated at build time and git-ignored. It lived at
common/static/xmodule/webpack.xmodule.config.js. It was generated
because the JS that it referred to was also generated at build-time, and
the filenames of those JS modules were not static.

Now that its contents have been made entirely static [1], there is no
reason we need to continue generating this Webpack configuration file.
So, we check it into edx-platform under the name
./webpack.builtinblocks.config.js. We choose to put it in the repo's
root directory because the paths contained in the config file are
relative to the repo's root.

This allows us to behead both the xmodule/static_content.py
(`xmodule_assets`) script andthe  `process_xmodule_assets` paver task, a
major step in removing the need for Python in the edx-platform asset
build [2]. It also allows us to delete the `HTMLSnippet` class and all
associated attributes, which were exclusively used by
xmodule/static_content.py..

We leave `xmodule_assets` and  `process_xmodule_assets` in as stubs for
now in order to avoid breaking external code (like Tutor) which calls
Paver; the entire pavelib/assets.py function will be eventually removed
soon anyway [3]. Further, to avoid extraneous refactoring, we keep one
method of `HTMLSnippet` around on a few of its former subclasses:
`get_html`. This method was originally part of the XModule framework;
now, it is left over on a few classes as a simple internal helper
method.

References:
1. #32480
2. #31800
3. #31895

Part of: #32481
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