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

Radically simplify XModule SCSS #32283

Closed
wants to merge 6 commits into from
Closed

Radically simplify XModule SCSS #32283

wants to merge 6 commits into from

Conversation

kdmccormick
Copy link
Member

Description

Overview

Details

See commit log

Supporting info

#31624

Test instructions

TBD

@kdmccormick kdmccormick force-pushed the kdmccormick/xmodule-scss branch from 7850e7c to acc6dd9 Compare May 23, 2023 02:59
@kdmccormick
refactor: define resource paths (not contents) on XModule classes
4b1b2dd 
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
04d503e 
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 kdmccormick force-pushed the kdmccormick/xmodule-scss branch from acc6dd9 to f5c5930 Compare May 23, 2023 14:42
@kdmccormick
build: stop suffixing XModule SCSS with hashes
97465e3 
Similar to openedx#32287,
this change removes another unnecessary step from the
`xmodule_assets` script. The script, which generates XModule
SCSS "entrypoint" files (synthesizing one or more "source" SCSS
resources), was appending MD5 hashes to each SCSS entrypoint filename:

    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

It's unclear why these MD5 hashes needed to be appended.
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 SCSS entrypoint files (one for
studio_view and one for other student/author_views) and none of those
entrypoint files can possibly be shared between XModules.

Soon, as part of deleting the `xmodule_assets` script,
we would like to just check these SCSS files into version control
rather than generating them. In order to do that, we will need to
drop the hashes.

So, in the interest of isolating changes in hard-to-test subsystems,
this change simply drops the MD5 hashes from the generated SCSS
filenames. 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 kdmccormick force-pushed the kdmccormick/xmodule-scss branch from f5c5930 to 3ec7151 Compare May 23, 2023 15:32
@kdmccormick
build: import XModule source SCSS directly rather than copying
66e6f4e 
The `xmodule_assets` command copies SCSS source files from
xmodule/css to common/static/xmodule/scss, renaming them
to `{MD5_HASH}.scss` in order to "remove duplicates".
The copied files are then included into the generated
SCSS entrypoint files (eg AnnotatableBlockStudio.scss).

The "de-deplication" is completely unnecessary: there are
only a couple dozen SCSS files, and none of them are duplicates.
This copying process is confusing, it complicates our
build process, and it makes our SCSS harder to understand.

So, in the generated SCSS entrypoint files, we
stop importing the *copied* SCSS sources, and just
import the *original* SCSS sources instead.
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 `@import` lines work, we add xmodule/css to the list
of lookup dirs for XModule SCSS compilation. We also remove the
copying logic from `xmodule_assets`, as it is no longer needed.

Part of: openedx#31624
@kdmccormick
build: check in XModule SCSS entry files instead of generating them
fa043aa 
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
build: common/static/css/xmodule -> xmodule/static/css
6a1038a 
Part of: openedx#31624
@kdmccormick kdmccormick force-pushed the kdmccormick/xmodule-scss branch from 3ec7151 to 6a1038a Compare May 23, 2023 15:40
@kdmccormick
Copy link
Member Author

Closed in favor of a series of finer-grained PRs, starting with: #32286

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