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

revert: build: commit builtinblocks Webpack config and stub out xmodule_assets #32867

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lms/djangoapps/courseware/tests/test_discussion_xblock.py
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ def test_has_permission(self):
"""
permission_canary = object()
with mock.patch(
'xmodule.discussion_block.has_permission',
'lms.djangoapps.discussion.django_comment_client.permissions.has_permission',
return_value=permission_canary,
) as has_perm:
actual_permission = self.block.has_permission("test_permission")
Expand Down
54 changes: 51 additions & 3 deletions pavelib/assets.py
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,7 @@ class SassWatcher(PatternMatchingEventHandler):
"""
ignore_directories = True
patterns = ['*.scss']
ignore_patterns = ['common/static/xmodule/*']

def register(self, observer, directories):
"""
Expand Down Expand Up @@ -351,6 +352,47 @@ 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([
Expand Down Expand Up @@ -573,13 +615,16 @@ def process_npm_assets():


@task
@needs(
'pavelib.prereqs.install_python_prereqs',
)
@no_help
def process_xmodule_assets():
"""
Process XModule static 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.")
sh('xmodule_assets common/static/xmodule')
print("\t\tFinished processing xmodule assets.")


def restart_django_servers():
Expand Down Expand Up @@ -777,6 +822,8 @@ 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()
Expand All @@ -798,7 +845,6 @@ def watch_assets(options):
@task
@needs(
'pavelib.prereqs.install_node_prereqs',
'pavelib.prereqs.install_python_prereqs',
)
@consume_args
@timed
Expand Down Expand Up @@ -850,6 +896,8 @@ def update_assets(args):
args = parser.parse_args(args)
collect_log_args = {}

process_xmodule_assets()

# Build Webpack
call_task('pavelib.assets.webpack', options={'settings': args.settings})

Expand Down
1 change: 1 addition & 0 deletions pavelib/js_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
@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"),
Expand Down
2 changes: 2 additions & 0 deletions pavelib/paver_tests/test_servers.py
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,7 @@ 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,
Expand Down Expand Up @@ -258,6 +259,7 @@ 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,
Expand Down
136 changes: 0 additions & 136 deletions webpack.builtinblocks.config.js

This file was deleted.

4 changes: 2 additions & 2 deletions webpack.common.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ var StringReplace = require('string-replace-webpack-plugin');
var Merge = require('webpack-merge');

var files = require('./webpack-config/file-lists.js');
var builtinBlocksJS = require('./webpack.builtinblocks.config.js');
var xmoduleJS = require('./common/static/xmodule/webpack.xmodule.config.js');

var filesWithRequireJSBlocks = [
path.resolve(__dirname, 'common/static/common/js/components/utils/view_utils.js'),
Expand Down Expand Up @@ -553,4 +553,4 @@ module.exports = Merge.smart({
}

}
}, {web: builtinBlocksJS}, workerConfig());
}, {web: xmoduleJS}, workerConfig());
19 changes: 19 additions & 0 deletions xmodule/annotatable_block.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import textwrap

from lxml import etree
from pkg_resources import resource_filename
from web_fragments.fragment import Fragment
from xblock.core import XBlock
from xblock.fields import Scope, String
Expand All @@ -14,6 +15,7 @@
from xmodule.util.builtin_assets import add_webpack_js_to_fragment, add_sass_to_fragment
from xmodule.xml_block import XmlMixin
from xmodule.x_module import (
HTMLSnippet,
ResourceTemplates,
shim_xmodule_js,
XModuleMixin,
Expand All @@ -33,6 +35,7 @@ class AnnotatableBlock(
XmlMixin,
EditingMixin,
XModuleToXBlockMixin,
HTMLSnippet,
ResourceTemplates,
XModuleMixin,
):
Expand Down Expand Up @@ -70,6 +73,22 @@ class AnnotatableBlock(

uses_xmodule_styles_setup = True

preview_view_js = {
'js': [
resource_filename(__name__, 'js/src/html/display.js'),
resource_filename(__name__, 'js/src/annotatable/display.js'),
resource_filename(__name__, 'js/src/javascript_loader.js'),
resource_filename(__name__, 'js/src/collapsible.js'),
],
'xmodule_js': resource_filename(__name__, 'js/src/xmodule.js'),
}

studio_view_js = {
'js': [
resource_filename(__name__, 'js/src/raw/edit/xml.js'),
],
'xmodule_js': resource_filename(__name__, 'js/src/xmodule.js'),
}
studio_js_module_name = "XMLEditingDescriptor"
mako_template = "widgets/raw-edit.html"

Expand Down
13 changes: 8 additions & 5 deletions xmodule/assets/README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@ the corresponding folders in any enabled themes, as part of the edx-platform bui
It is collected into the static root, and then linked to from XBlock fragments by the
``add_sass_to_fragment`` function in `builtin_assets.py`_.

.. _AnnotatableBlockDisplay.scss: https://github.com/openedx/edx-platform/tree/master/xmodule/assets/AnnotatableBlockDisplay.scss
.. _AnnotatableBlockEditor.scss: https://github.com/openedx/edx-platform/tree/master/xmodule/assets/AnnotatableBlockEditor.scss
.. _AnnotatableBlockDisplay: https://github.com/openedx/edx-platform/tree/master/xmodule/assets/AnnotatableBlockDisplay.scss
.. _AnnotatableBlockEditor: https://github.com/openedx/edx-platform/tree/master/xmodule/assets/AnnotatableBlockEditor.scss
.. _annotatable/_display.scss: https://github.com/openedx/edx-platform/tree/master/xmodule/assets/annotatable/_display.scss
.. _simplify things: https://github.com/openedx/edx-platform/issues/32621

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

* For many older blocks, their JS is:

* bundled using a `webpack.builtinblocks.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 @@ -74,7 +74,11 @@ Currently, edx-platform XBlock JS is defined both here in `xmodule/assets`_ and
* `VerticalBlock`_
* `LibrarySourcedBlock`_

As part of an `active build refactoring`_, we will soon consolidate all edx-platform XBlock JS here in `xmodule/assets`_.
As part of an `active build refactoring`_:

* 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.

.. _xmodule/assets: https://github.com/openedx/edx-platform/tree/master/xmodule/assets
.. _xmodule/js: https://github.com/openedx/edx-platform/tree/master/xmodule/js
Expand All @@ -87,5 +91,4 @@ As part of an `active build refactoring`_, we will soon consolidate all edx-plat
.. _builtin_assets.py: https://github.com/openedx/edx-platform/tree/master/xmodule/util/builtin_assets.py
.. _static_content.py: https://github.com/openedx/edx-platform/blob/master/xmodule/static_content.py
.. _library_source_block/style.css: https://github.com/openedx/edx-platform/blob/master/xmodule/assets/library_source_block/style.css
.. _webpack.builtinblocks.config.js: https://github.com/openedx/edx-platform/blob/master/webpack.builtinblocks.config.js
.. _webpack.common.config.js: https://github.com/openedx/edx-platform/blob/master/webpack.common.config.js
Loading