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.

Sign up for GitHub

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

Jump to bottom

Stop dynamically generating XModule JS #32481

Closed
3 tasks done
Tracked by #31624
, #31800
...
kdmccormick opened this issue Jun 15, 2023 · 1 comment
Closed
3 tasks done
Tracked by #31624
, #31800
...

Stop dynamically generating XModule JS #32481

kdmccormick opened this issue Jun 15, 2023 · 1 comment
Assignees
@kdmccormick

Comments

@kdmccormick
Copy link
Member

kdmccormick commented Jun 15, 2023 •
edited
Loading

Background

Goal

All XModule JS should be committed to the repository rather than generated as part of the build process. Benefits:

  • Delete xmodule/static_content.py (aka xmodule_assets), unblocking us from removing Python from the build process.
  • Make XModule assets more similar to standard XBlock assets.
  • Make it so XModule JS can be read by a human being; no tooling or special context necessary.

PRs
Beta Give feedback
  1. [1/3] refactor: remove XModule JS from Django Pipeline #32530
  2. [2/3] build: include built-in XBlock JS directly rather than copying it #32480
  3. [3/3] build: commit builtinblocks Webpack config and stub out xmodule_assets #32685
@kdmccormick kdmccormick mentioned this issue Jun 15, 2023
Closed
@kdmccormick kdmccormick self-assigned this Jun 15, 2023
@kdmccormick kdmccormick mentioned this issue Jun 16, 2023
Merged
@kdmccormick kdmccormick mentioned this issue Jul 6, 2023
Merged
kdmccormick added a commit to kdmccormick/edx-platform that referenced this issue Jul 7, 2023
@kdmccormick
refactor: remove XModule JS from Django Pipeline
17faca5 
TODO - I will update this commit message from the PR description when
squashing.

Part of: openedx#32481
This was referenced Jul 7, 2023
Merged
Merged
Open
kdmccormick added a commit to kdmccormick/edx-platform that referenced this issue Jul 7, 2023
@kdmccormick
build: include built-in XBlock JS directly rather than copying it
5391623 
TODO: will copy in commit message from PR when squash merging

Part of: openedx#32481
kdmccormick added a commit that referenced this issue Jul 18, 2023
@kdmccormick
refactor: remove XModule JS from Django Pipeline (#32530)
4aedeb8 
`module-js` and `module-descriptor-js` are old JavaScript group
indicators, left over from when we managed XModule assets via Django
Pipeline. We would like to get rid of them in order to make it easier to
build XModule JS without using Python.

There is one single usage of `module-js` in the entire platform (the
rest have been replaced with Webpack references, which is the
less-outdated way of managing XModule assets :). The lone `module-js`
reference was added in 2013 [1] so that circuit diagrams would display
in the course wiki. However, the ability to render circuits in the wiki
was removed in 2015 [2], so it is safe to remove the reference.

There is also one single usage of `module-descriptor-js`. It's in the
legacy bulk email editor, which hackily cribs from the old HtmlBlock
editor. Fortunately, we are able to simply replace the Django Pipeline
reference with the equivalent XModule JS Webpack bundle. (Note: The old
email editor is currently still supported, but is currently being
replaced by frontend-app-communications, so this hack will be gone
eventually).

Finally, this commit also sneaks in one styling fix: it adds the
HtmlBlockEditor CSS back to the aforementioned legacy bulk email page.
The missing CSS was causing a read-only 1-line codemirror editor to
appear below the HTML editor [3]. This bug was introduced during the
original XModule SCSS decoupling [4], which removed builtin block CSS
from the LMS-wide bundle, thus removing the HTML editor CSS from the
bulk email page. We imagine that nobody noticed because the bug only
exists in master (not Palm) and frontend-app-communications seems to be
globally enabled on edx.org. As a simple fix, we add the new CSS link to
the legacy bulk email page, and it renders fine again [5].

References:

1. 3fc59b3
2. #10324
3. Before fix: https://github.com/openedx/edx-platform/assets/3628148/25fc41b2-403d-4339-8c49-0b04664dfa02
4. #32018
5. After fix: https://github.com/openedx/edx-platform/assets/3628148/9a5d74f1-cc83-4ebe-8f0c-ee270f7721b8

Part of: #32481
kdmccormick added a commit to kdmccormick/edx-platform that referenced this issue Jul 18, 2023
@kdmccormick
build: include built-in XBlock JS directly rather than copying it
ead1eef 
TODO: will copy in commit message from PR when squash merging

Part of: openedx#32481
kdmccormick added a commit to kdmccormick/edx-platform that referenced this issue Jul 18, 2023
@kdmccormick
build: commit xmodule Webpack config and delete xmodule_assets script
38cca73 
TODO: will copy in commit message from PR when squash-merging

Part of: openedx#32481
kdmccormick added a commit to kdmccormick/edx-platform that referenced this issue Jul 26, 2023
@kdmccormick
build: include built-in XBlock JS directly rather than copying it
e87b57d 
TODO: will copy in commit message from PR when squash merging

Part of: openedx#32481
kdmccormick added a commit that referenced this issue Jul 26, 2023
@kdmccormick
build: include built-in XBlock JS directly rather than copying it (#3…
9d4163d 
…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
kdmccormick added a commit to kdmccormick/edx-platform that referenced this issue Jul 27, 2023
@kdmccormick
build: commit builtinblocks Webpack config and stub out xmodule_assets
7eb4836 
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 issue Jul 27, 2023
@kdmccormick
build: commit builtinblocks Webpack config and stub out `xmodule_asse…
3557799 
…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
@kdmccormick
Copy link
Member Author

This is done!

kdmccormick added a commit to kdmccormick/edx-platform that referenced this issue Aug 12, 2024
@kdmccormick
refactor: remove unused builtinblock webpack entry points
775fd28 
Most blocks built into edx-platform have a pair of webpack entry
points, like this:

* {BlockName}Display    # js for student+author+public views
* {BlockName}Editor     # js for studio view

Prior to a past build refactoring [1], these entry points were
defined in an auto-genered webpack config file. In order
to simplify the js build process, this config generation
step was removed and the new file webpack.builtinblocks.config.js
was checked directly into edx-platform.

However, during that refactoring, three paris of pointless entry
points were introduced:

* AboutBlockDisplay, AboutBlockEditor
* CourseInfoBlockDisplay, CourseInfoBlockEditor
* StaticTabBlockDisplay, StaticTabBlockEditor

They are pointless because those three XBlocks are just subclasses
of HtmlBlock, and they use HtmlBlock's JS: HtmlBlockDisplay and
HtmlBlockEditor. So, we delete them from webpack.builtinblocks.js,
which will have no effect on anything but may help avoid dev
confusion in the future.

[1] openedx#32481
@kdmccormick kdmccormick mentioned this issue Aug 12, 2024
Merged
kdmccormick added a commit that referenced this issue Aug 20, 2024
@kdmccormick
refactor: remove unused builtinblock webpack entry points (#35299)
688242a 
Most blocks built into edx-platform have a pair of webpack entry
points, like this:

* {BlockName}Display    # js for student+author+public views
* {BlockName}Editor     # js for studio view

Prior to a past build refactoring [1], these entry points were
defined in an auto-genered webpack config file. In order
to simplify the js build process, this config generation
step was removed and the new file webpack.builtinblocks.config.js
was checked directly into edx-platform.

However, during that refactoring, pointless entry
points were introduced for HtmlBlock subclasses
About, CourseInfo, and StaticTab, all of which
just use their superclass's JS.

[1] #32481
rediris pushed a commit to gymnasium/edx-platform that referenced this issue Aug 27, 2024
@kdmccormick @rediris
refactor: remove unused builtinblock webpack entry points (openedx#35299
5864601 
)

Most blocks built into edx-platform have a pair of webpack entry
points, like this:

* {BlockName}Display    # js for student+author+public views
* {BlockName}Editor     # js for studio view

Prior to a past build refactoring [1], these entry points were
defined in an auto-genered webpack config file. In order
to simplify the js build process, this config generation
step was removed and the new file webpack.builtinblocks.config.js
was checked directly into edx-platform.

However, during that refactoring, pointless entry
points were introduced for HtmlBlock subclasses
About, CourseInfo, and StaticTab, all of which
just use their superclass's JS.

[1] openedx#32481
@kdmccormick kdmccormick mentioned this issue Oct 15, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kdmccormick kdmccormick

Labels
None yet
Projects
No open projects
Developer Experience Working Group
Status: Closed
Milestone
No milestone
Development

No branches or pull requests
1 participant
@kdmccormick