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

[3/3] build: commit builtinblocks Webpack config and stub out xmodule_assets #32685

Merged
merged 2 commits into from
Jul 27, 2023

Conversation

kdmccormick
Copy link
Member

@kdmccormick kdmccormick commented Jul 7, 2023

Part of a series of PRs:

The previous one is:

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

Description and supporting info

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. [2/3] build: include built-in XBlock JS directly rather than copying it #32480
  2. Build edx-platform assets without Python #31800
  3. [DEPR]: Asset processing in Paver #31895

Testing

Follow the same instructions from #32480, but sub in this branch.

@kdmccormick kdmccormick changed the title [3/3] build: commit XModule Webpack config and delete xmodule_assets script [3/3] build: commit xmodule Webpack config and delete xmodule_assets script Jul 7, 2023
@kdmccormick kdmccormick force-pushed the kdmccormick/xmodule-js-commit branch 5 times, most recently from 1f13076 to ff3ab4d Compare July 8, 2023 01:07
@kdmccormick kdmccormick marked this pull request as ready for review July 10, 2023 21:26
@kdmccormick kdmccormick force-pushed the kdmccormick/xmodule-js-commit branch 2 times, most recently from 6666750 to f93ab1d Compare July 11, 2023 15:21
@kdmccormick kdmccormick marked this pull request as draft July 11, 2023 18:57
@kdmccormick kdmccormick force-pushed the kdmccormick/xmodule-js-commit branch 2 times, most recently from 38cca73 to 6b38d03 Compare July 21, 2023 02:40
@kdmccormick kdmccormick force-pushed the kdmccormick/xmodule-js-commit branch 2 times, most recently from 9cf9845 to 26ee588 Compare July 25, 2023 14:57
@kdmccormick kdmccormick changed the title [3/3] build: commit xmodule Webpack config and delete xmodule_assets script [3/3] build: commit xmodule Webpack config and stub out xmodule_assets script Jul 25, 2023
@kdmccormick kdmccormick force-pushed the kdmccormick/xmodule-js-commit branch 2 times, most recently from d427724 to 02d90e3 Compare July 25, 2023 15:10
@kdmccormick kdmccormick changed the title [3/3] build: commit xmodule Webpack config and stub out xmodule_assets script [3/3] build: commit builtinblocks Webpack config and stub out xmodule_assets Jul 25, 2023
@kdmccormick kdmccormick force-pushed the kdmccormick/xmodule-js-commit branch from 02d90e3 to 04a4eda Compare July 25, 2023 15:24
@kdmccormick kdmccormick marked this pull request as ready for review July 25, 2023 15:26
@kdmccormick kdmccormick force-pushed the kdmccormick/xmodule-js-commit branch from 04a4eda to fad86dc Compare July 25, 2023 15:38
kdmccormick added a commit that referenced this pull request Jul 26, 2023
This PR implements much of the static assets rework ADR [1], including:

* `npm run build[-dev]`, and its subcommands,
* `npm run webpack[-dev]` and
* `npm run compile-sass[-dev]`.

This is backwards-compatible. `paver update_assets` should not be affected.
The new command warns that it is "experimental" for a few reasons:

* `npm run build` will fail in the webpack phase unless you first
run  `xmodule_assets`. This will be changed soon [2].

* We have tested the new build, but not quite so thoroughly that we'd
recommend it as the production default yet. Once the xmodule_assets
work lands, we'll share this on the forums so early adopters can try it
out.

* The commands lack some top-level documentation. Once they stabilize more,
we'll add a section to the README that explains how and when to use `npm run
build` and its subcommands and its env vars.

* `npm run watch` is not yet implemented.

References:
1. https://github.com/openedx/edx-platform/blob/master/docs/decisions/0017-reimplement-asset-processing.rst
2. #32685

Part of: #31604
@kdmccormick kdmccormick force-pushed the kdmccormick/xmodule-js-commit branch from fad86dc to 73f28cc Compare July 26, 2023 21:40
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 kdmccormick force-pushed the kdmccormick/xmodule-js-commit branch from 73f28cc to 7eb4836 Compare July 27, 2023 13:26
@kdmccormick kdmccormick enabled auto-merge (squash) July 27, 2023 14:15
@kdmccormick kdmccormick merged commit 3557799 into openedx:master Jul 27, 2023
41 checks passed
@kdmccormick kdmccormick deleted the kdmccormick/xmodule-js-commit branch July 27, 2023 14:33
kdmccormick added a commit that referenced this pull request Jul 27, 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.

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