Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

build: include built-in XBlock JS directly rather than copying it (#32480)

Browse Source 
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: https://github.com/openedx/edx-platform/issues/32481
This commit is contained in:
Kyle McCormick
2023-07-26 13:27:38 -04:00
committed by GitHub
parent 51079e5cf3
commit 9d4163d31f
6 changed files with 174 additions and 135 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
conf/locale/config.yaml
View File

@@ -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'

4
webpack-config/file-lists.js
View File

@@ -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')
],

118
webpack.common.config.js
View File

@@ -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|\$)(, )?)+\) ?\{/;
@@ -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'

4
xmodule/assets/README.rst
View File

@@ -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`_.
@@ -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.

168
xmodule/static_content.py
View File

@@ -1,22 +1,48 @@
# /usr/bin/env python
"""
This module has utility functions for gathering up the javascript
that is defined by XModules and XModuleDescriptors
Generate <output_root>/webpack.xmodule.config.js, with a display & editor Webpack bundle for each builtin block.
It looks like this:
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"
],
"AnnotatableBlockDisplay": [
"./xmodule/js/src/xmodule.js",
"./xmodule/js/src/html/display.js",
"./xmodule/js/src/annotatable/display.js",
"./xmodule/js/src/javascript_loader.js",
"./xmodule/js/src/collapsible.js"
],
... etc.
}
}
Don't add to this! It will soon be removed as part of: https://github.com/openedx/edx-platform/issues/32481
"""
import errno
import hashlib
import json
import logging
import os
import sys
import textwrap
from collections import defaultdict
from pkg_resources import resource_filename
import django
from path import Path as path
from pathlib import Path as path
from xmodule.annotatable_block import AnnotatableBlock
from xmodule.capa_block import ProblemBlock
@@ -76,16 +102,6 @@ XBLOCK_CLASSES = [
]
def write_module_js(output_root):
"""Write all registered XModule js and coffee files to output root."""
return _write_js(output_root, XBLOCK_CLASSES, 'get_preview_view_js')
def write_descriptor_js(output_root):
"""Write all registered XModuleDescriptor js and coffee files to output root."""
return _write_js(output_root, XBLOCK_CLASSES, 'get_studio_view_js')
def _ensure_dir(directory):
"""Ensure that `directory` exists."""
try:
@@ -97,110 +113,21 @@ def _ensure_dir(directory):
raise
def _write_js(output_root, classes, js_attribute):
"""
Write the javascript fragments from all XModules in `classes`
into `output_root` as individual files, hashed by the contents to remove
duplicates
Returns a dictionary mapping class names to the files that they depend on.
"""
file_contents = {}
file_owners = defaultdict(list)
fragment_owners = defaultdict(list)
for class_ in classes:
module_js = getattr(class_, js_attribute)()
with open(module_js.get('xmodule_js'), 'rb') as xmodule_js_file:
xmodule_js_fragment = xmodule_js_file.read()
# It will enforce 000 prefix for xmodule.js.
fragment_owners[(0, 'js', xmodule_js_fragment)].append(getattr(class_, js_attribute + '_bundle_name')())
for filetype in ('coffee', 'js'):
for idx, fragment_path in enumerate(module_js.get(filetype, [])):
with open(fragment_path, 'rb') as fragment_file:
fragment = fragment_file.read()
fragment_owners[(idx + 1, filetype, fragment)].append(getattr(class_, js_attribute + '_bundle_name')())
for (idx, filetype, fragment), owners in sorted(fragment_owners.items()):
filename = "{idx:0=3d}-{hash}.{type}".format(
idx=idx,
hash=hashlib.md5(fragment).hexdigest(),
type=filetype)
file_contents[filename] = fragment
for owner in owners:
file_owners[owner].append(output_root / filename)
_write_files(output_root, file_contents, {'.coffee': '.js'})
return file_owners
def _write_files(output_root, contents, generated_suffix_map=None):
"""
Write file contents to output root.
Any files not listed in contents that exists in output_root will be deleted,
unless it matches one of the patterns in `generated_suffix_map`.
output_root (path): The root directory to write the file contents in
contents (dict): A map from filenames to file contents to be written to the output_root
generated_suffix_map (dict): Optional. Maps file suffix to generated file suffix.
For any file in contents, if the suffix matches a key in `generated_suffix_map`,
then the same filename with the suffix replaced by the value from `generated_suffix_map`
will be ignored
"""
_ensure_dir(output_root)
to_delete = {file.basename() for file in output_root.files()} - set(contents.keys())
if generated_suffix_map:
for output_file in contents.keys():
for suffix, generated_suffix in generated_suffix_map.items():
if output_file.endswith(suffix):
to_delete.discard(output_file.replace(suffix, generated_suffix))
for extra_file in to_delete:
(output_root / extra_file).remove_p()
for filename, file_content in contents.items():
output_file = output_root / filename
not_file = not output_file.isfile()
# Sometimes content is already unicode and sometimes it's not
# so we add this conditional here to make sure that below we're
# always working with streams of bytes.
if not isinstance(file_content, bytes):
file_content = file_content.encode('utf-8')
# not_file is included to short-circuit this check, because
# read_md5 depends on the file already existing
write_file = not_file or output_file.read_md5() != hashlib.md5(file_content).digest()
if write_file:
LOG.debug("Writing %s", output_file)
output_file.write_bytes(file_content)
else:
LOG.debug("%s unchanged, skipping", output_file)
def write_webpack(output_file, module_files, descriptor_files):
"""
Write all xmodule and xmodule descriptor javascript into module-specific bundles.
The output format should be suitable for smart-merging into an existing webpack configuration.
"""
_ensure_dir(output_file.dirname())
_ensure_dir(output_file.parent)
config = {
'entry': {}
}
for (owner, files) in list(module_files.items()) + list(descriptor_files.items()):
unique_files = sorted({f'./{file}' for file in files})
for (owner, unique_files) in list(module_files.items()) + list(descriptor_files.items()):
if len(unique_files) == 1:
unique_files = unique_files[0]
config['entry'][owner] = unique_files
# config['entry']['modules/js/all'] = sorted(set('./{}'.format(file) for file in sum(module_files.values(), [])))
# config['entry']['descriptors/js/all'] = sorted(set('./{}'.format(file) for file in sum(descriptor_files.values(), []))) # lint-amnesty, pylint: disable=line-too-long
with output_file.open('w') as outfile:
outfile.write(
textwrap.dedent("""\
@@ -217,7 +144,8 @@ def write_webpack(output_file, module_files, descriptor_files):
def main():
"""
Generate
Generate the weback config.
Usage: static_content.py <output_root>
"""
from django.conf import settings
@@ -245,8 +173,30 @@ def main():
except IndexError:
sys.exit(main.__doc__)
descriptor_files = write_descriptor_js(root / 'descriptors/js')
module_files = write_module_js(root / 'modules/js')
# We assume this module is located at edx-platform/xmodule/static_content.py.
# Not the most robust assumption, but this script will be gone soon.
repo_root = path(__file__).parent.parent
module_files = {
class_.get_preview_view_js_bundle_name(): [
"./" + str(path(fragment_path).relative_to(repo_root))
for fragment_path in [
class_.get_preview_view_js()['xmodule_js'],
*class_.get_preview_view_js().get('js', []),
]
]
for class_ in XBLOCK_CLASSES
}
descriptor_files = {
class_.get_studio_view_js_bundle_name(): [
"./" + str(path(fragment_path).relative_to(repo_root))
for fragment_path in [
class_.get_studio_view_js()['xmodule_js'],
*class_.get_studio_view_js().get('js', []),
]
]
for class_ in XBLOCK_CLASSES
}
write_webpack(root / 'webpack.xmodule.config.js', module_files, descriptor_files)

13
xmodule/tests/test_content.py
View File

@@ -1,17 +1,14 @@
"""Tests for contents"""
import os
import unittest
from unittest.mock import Mock, patch
import ddt
from opaque_keys.edx.keys import CourseKey
from opaque_keys.edx.locator import AssetLocator, CourseLocator
from path import Path as path
from xmodule.contentstore.content import ContentStore, StaticContent, StaticContentStream
from xmodule.static_content import XBLOCK_CLASSES, _write_js
SAMPLE_STRING = """
This is a sample string with more than 1024 bytes, the default STREAM_DATA_CHUNK_SIZE
@@ -205,13 +202,3 @@ class ContentTest(unittest.TestCase): # lint-amnesty, pylint: disable=missing-c
total_length += len(chunck)
assert total_length == ((last_byte - first_byte) + 1)
def test_static_content_write_js(self):
"""
Test that only one filename starts with 000.
"""
output_root = path('common/static/xmodule/descriptors/js')
file_owners = _write_js(output_root, XBLOCK_CLASSES, 'get_studio_view_js')
js_file_paths = {file_path for file_path in sum(list(file_owners.values()), []) if os.path.basename(file_path).startswith('000-')} # lint-amnesty, pylint: disable=line-too-long
assert len(js_file_paths) == 1
assert 'XModule.Descriptor = (function() {' in open(js_file_paths.pop()).read()