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

Kdmccormick/xmodule assets #32272

Closed
wants to merge 11 commits into from
Closed

Kdmccormick/xmodule assets #32272

wants to merge 11 commits into from

Conversation

kdmccormick
Copy link
Member

WIP

Part of #31624

Description

Describe what this pull request changes, and why. Include implications for people using this change.
Design decisions and their rationales should be documented in the repo (docstring / ADR), per
OEP-19, and can be
linked here.

Useful information to include:

  • Which edX user roles will this change impact? Common user roles are "Learner", "Course Author",
    "Developer", and "Operator".
  • Include screenshots for changes to the UI (ideally, both "before" and "after" screenshots, if applicable).
  • Provide links to the description of corresponding configuration changes. Remember to correctly annotate these
    changes.

Supporting information

Link to other information about the change, such as Jira issues, GitHub issues, or Discourse discussions.
Be sure to check they are publicly readable, or if not, repeat the information here.

Testing instructions

Please provide detailed step-by-step instructions for testing this change.

Deadline

"None" if there's no rush, or provide a specific date or event (and reason) if there is one.

Other information

Include anything else that will help reviewers and consumers understand the change.

  • Does this change depend on other changes elsewhere?
  • Any special concerns or limitations? For example: deprecations, migrations, security, or accessibility.
  • If your database migration can't be rolled back easily.

@kdmccormick kdmccormick force-pushed the kdmccormick/xmodule-assets branch 2 times, most recently from 3961c75 to 096d799 Compare May 19, 2023 17:23
@kdmccormick kdmccormick mentioned this pull request May 19, 2023
Closed
@kdmccormick kdmccormick force-pushed the kdmccormick/xmodule-assets branch 3 times, most recently from 7afd21b to e54778e Compare May 22, 2023 21:28
@kdmccormick
refactor: define resource paths (not contents) on XModule classes
ece1dbd 
For the XBlocks types that use legacy XModule-style assets, this is small
refactor that brings them a bit closer to being like XBlocks.

Given class attributes on those block types in the form:

    SomeXModuleLikeBlock.(studio|preview)_view_(js|css)['(js|scss|css|xmodule_js)']

we make it so their value is the *path to the resource*
rather than *the actual content of the resource*.

This will make future refactorings simpler.

Part of: openedx#31624
@kdmccormick
build: stop prefixing copied XModule SCSS with indexes
a5e1e8c 
The `xmodule_assets` command copies SCSS files from
xmodule/css to common/static/xmodule/{modules|descriptors}/scss.
It renames the files to the format:

   _{INDEX}-{HASH}.scss

where an XModule's first SCSS resource will have INDEX==0,
the next will have INDEX==1, ...and that's it because no
XModule has more than two SCSS resources.

The output looks like this:

  common/static/xmodule/descriptors/scss:
    _000-808fcbb4c5109c5156ae3c0c9729c8be.scss
    _000-9bdcda00f046f78be79aca7791e1d4fb.scss
    _000-d41921b4c5d45188759ef3d04fd9a78a.scss
    _001-901b985e5ea2dea2a89cce747cf4307d.scss
    _001-a10fc3e0fd6aca63426a89e75fe69c31.scss
  common/static/xmodule/modules/scss:
    _000-1ad2f05db822d3176affd203d70319c0.scss
    _000-1dc4276d3849a14ea538286e97740c14.scss
    _000-29baf1ef1af89b1051362f51124abd01.scss
    _000-6bf8c2340b013d835b25df13e03b8d33.scss
    _000-8b6bb50b058d34efefa40107307a32c6.scss
    _000-958d6ef6baa09be94bccaf488861c8e5.scss
    _000-a3c2cdf2141d24a76be9afa56f237c29.scss
    _000-b80300e1a5f290f6a850e35874068427.scss
    _001-482ebc752ab6e41946651ceb0f3e7f55.scss

These indexes serve no purpose. Reading the comments
and git-blame in xmodule/static_content.py, one can glean
that they indexes might have been intended to enforce
dependency relationships between the assets, but
this is unnecessary, because the ordering of the copied
SCSS is *already preserved* by the order which they're
included into the {BLOCK_NAME}{Studio|Preivew}.{HASH}.scss
SCSS entrypoint files. I have to assume that this is an
unnecessary relic from the timebefore the XModule system was
reduced to a legacy corner of the XBlock framework.

So, we remove the indexes, which lets us simplify the logic
of xmodule/static_content.py. This is a minor refactoring, but it'll
make it easier for the next steps on our way to deleting
xmodule/static_content.py entirely. The new output looks like this:

  common/static/xmodule/descriptors/scss:
    _808fcbb4c5109c5156ae3c0c9729c8be.scss
    _901b985e5ea2dea2a89cce747cf4307d.scss
    _9bdcda00f046f78be79aca7791e1d4fb.scss
    _a10fc3e0fd6aca63426a89e75fe69c31.scss
    _d41921b4c5d45188759ef3d04fd9a78a.scss
  common/static/xmodule/modules/scss:
    _1ad2f05db822d3176affd203d70319c0.scss
    _1dc4276d3849a14ea538286e97740c14.scss
    _29baf1ef1af89b1051362f51124abd01.scss
    _482ebc752ab6e41946651ceb0f3e7f55.scss
    _6bf8c2340b013d835b25df13e03b8d33.scss
    _8b6bb50b058d34efefa40107307a32c6.scss
    _958d6ef6baa09be94bccaf488861c8e5.scss
    _a3c2cdf2141d24a76be9afa56f237c29.scss
    _b80300e1a5f290f6a850e35874068427.scss

Part of: openedx#31624
@kdmccormick
build: stop suffixing XModule SCSS with hashes
9dd7460 
Similar to the previous commit, this is another thing that
xmodule_assets does to its generated output that is completely
unnecessary. The XModule SCSS entrypoint files are suffixed with
MD5 hashes:

  common/static/xmodule/descriptors/scss:
     AboutBlockStudio.768623f4d8d73dfb637fc94583adb990.scss
     AnnotatableBlockStudio.be69909d83985d31e206fad272906958.scss
     ConditionalBlockStudio.d41d8cd98f00b204e9800998ecf8427e.scss
     CourseInfoBlockStudio.768623f4d8d73dfb637fc94583adb990.scss
     CustomTagBlockStudio.be69909d83985d31e206fad272906958.scss
     HtmlBlockStudio.768623f4d8d73dfb637fc94583adb990.scss
     LibraryContentBlockStudio.d41d8cd98f00b204e9800998ecf8427e.scss
     LTIBlockStudio.d41d8cd98f00b204e9800998ecf8427e.scss
     PollBlockStudio.d41d8cd98f00b204e9800998ecf8427e.scss
     ProblemBlockStudio.5893b30426f88e14712556c6c4342f23.scss
     SequenceBlockStudio.d41d8cd98f00b204e9800998ecf8427e.scss
     SplitTestBlockStudio.d41d8cd98f00b204e9800998ecf8427e.scss
     StaticTabBlockStudio.768623f4d8d73dfb637fc94583adb990.scss
     VideoBlockStudio.e4a6920a875dfb91eb65ee7e6dad7e2e.scss
     WordCloudBlockStudio.d41d8cd98f00b204e9800998ecf8427e.scss
  common/static/xmodule/modules/scss:
     AboutBlockPreview.05a6cbd5c10100a245fa2cbf151b9770.scss
     AnnotatableBlockPreview.7e95b106aa0a61824f4290da1374960d.scss
     ConditionalBlockPreview.d41d8cd98f00b204e9800998ecf8427e.scss
     CourseInfoBlockPreview.05a6cbd5c10100a245fa2cbf151b9770.scss
     CustomTagBlockPreview.d41d8cd98f00b204e9800998ecf8427e.scss
     HtmlBlockPreview.05a6cbd5c10100a245fa2cbf151b9770.scss
     LibraryContentBlockPreview.d41d8cd98f00b204e9800998ecf8427e.scss
     LTIBlockPreview.a763928b2c415251720f8634b8daee59.scss
     PollBlockPreview.39730e54c1eebbafd18a82fbb09c1e37.scss
     ProblemBlockPreview.70b905ac161108a0a03c639232450aaa.scss
     SequenceBlockPreview.e2336fa64ba495fa7c0f4f838d20ad8c.scss
     SplitTestBlockPreview.d41d8cd98f00b204e9800998ecf8427e.scss
     StaticTabBlockPreview.05a6cbd5c10100a245fa2cbf151b9770.scss
     VideoBlockPreview.b2de5e1c4da8cba16ce1c4bdeab50f9e.scss
     WordCloudBlockPreview.7b899a56a70d29c58cf14b7e1888a0ec.scss

A comment in xmodule/static_content.py hints that it might have
something to do with de-duplication, but that doesn't make any sense,
because each XModule has exactly two entrypoint files, and they aren't
shared.

So, we remove the hashes, as an intermediate step towards deleting
xmodule/static_content.py and instead just checking these SCSS
entrypoints directly into the repo. Until then, the new output
looks like this:

  common/static/xmodule/descriptors:
     AboutBlockStudio.scss
     AnnotatableBlockStudio.scss
     ConditionalBlockStudio.scss
     CourseInfoBlockStudio.scss
     CustomTagBlockStudio.scss
     HtmlBlockStudio.scss
     LibraryContentBlockStudio.scss
     LTIBlockStudio.scss
     PollBlockStudio.scss
     ProblemBlockStudio.scss
     SequenceBlockStudio.scss
     SplitTestBlockStudio.scss
     StaticTabBlockStudio.scss
     VideoBlockStudio.scss
     WordCloudBlockStudio.scss
  common/static/xmodule/modules:
     AboutBlockPreview.scss
     AnnotatableBlockPreview.scss
     ConditionalBlockPreview.scss
     CourseInfoBlockPreview.scss
     CustomTagBlockPreview.scss
     HtmlBlockPreview.scss
     LibraryContentBlockPreview.scss
     LTIBlockPreview.scss
     PollBlockPreview.scss
     ProblemBlockPreview.scss
     SequenceBlockPreview.scss
     SplitTestBlockPreview.scss
     StaticTabBlockPreview.scss
     VideoBlockPreview.scss
     WordCloudBlockPreview.scss

Part of: openedx#31624
@kdmccormick
build: import XModule source SCSS directly rather than copying
03b0d81 
The xmodule_assets command copies SCSS source files from
xmodule/css to common/static/xmodule/scss, renaming them
to {MD5_HASH}.scss to "remove duplicates" (this is completely
unnecessary; there are a couple dozen SCSS files and none
of them are duplicates). The copied files are then included
into generated SCSS entry files (eg AnnotatableBlockStudio.scss).

Rather than copying the original SCSS files, we change
the generate SCSS entrypoint files to simply import the
original SCSS files.

For example, common/static/xmodule/descriptors/scss/AboutBlockStudio.scss
is changed from:

    .xmodule_edit.xmodule_AboutBlock {
      @import "9bdcda00f046f78be79aca7791e1d4fb.scss";
      @import "a10fc3e0fd6aca63426a89e75fe69c31.scss";
    }

to:

    .xmodule_edit.xmodule_AboutBlock {
      @import "editor/edit.scss";
      @import "html/edit.scss";
    }

In order to make the @imports work, we add xmodule/css to the list
of lookup dirs for XModule SCSS compilation. Soon, we will
move the SCSS entry files to the same directory as the original
SCSS files, which will allow us to take xmodule/css back out of the list
of SCSS lookup dirs.

Part of: openedx#31624
@kdmccormick
build: check in XModule SCSS entry files instead of generating them
9867bc4 
xmodule_assets formerly generated a series of SCSS entry
files, which imported from SCSS sources in xmodule/static/sass.
This adds an unnecessary and confusing step to the SCSS build.

Instead, we move the generated files to xmodule/sass, and TODO

Part of: openedx#31624
@kdmccormick
style: add newline at end of XModule SCSS files
7f619e5 
Part of: openedx#31624
@kdmccormick
build: xmodule/sass -> xmodule/static/sass
17d3487 
Part of: openedx#31624
@kdmccormick
build: common/static/css/xmodule -> xmodule/static/css
85f6f0a 
Part of: openedx#31624
@kdmccormick
build: reference original XModule JS instead of copying
b727142 
TODO: details

Part of: openedx#31624
@kdmccormick
build: check in XModule webpack config; delete xmodule_assets script
dd25c77 
TODO:
* commit details
* understand PIPELINE['javascript']
* look for any other code that expects modules at old generated
  locations

Part of: openedx#31624
@kdmccormick
build: xmodule/js -> xmodule/static/js
ea3160d 
TODO:
* commit details
* understand PIPELINE['javascript']
* look for any other code that expects modules at
  xmodule/js instead of xmodule/static/js

Part of: openedx#31624
@kdmccormick
Copy link
Member Author

Closed for now in favor of #32292

Will come back to this branch when it's time to untangle the JS.

@kdmccormick kdmccormick deleted the kdmccormick/xmodule-assets branch July 27, 2023 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@kdmccormick