Remove special XModule asset handling #31624

kdmccormick · 2023-01-19T22:14:11Z

Tasks and PRs

Give feedback

Catalog XModule assets #31932
feat: move general imports to specific scss files #32121

open-source-contribution
Decouple XModule styles from LMS/Studio styles #32018

open-source-contribution
Decouple XModule styles from LMS/Studio styles (attempt 2) #32188

open-source-contribution
Decouple XModule styles from LMS/Studio styles (attempt 3) #32237

open-source-contribution
Stop dynamically generating XModule SCSS #32292

7 of 7
Stop dynamically generating XModule JS #32481

3 of 3
Options

Background

This issue helps towards several different initiatives:

edx-platform serves pure [1] XBlocks assets by implementing the standard Django Storage and Finder classes, which tells Django to looks for static assets within the resources_dir of all installed XBlock packages. Importantly, this finder filters out all py, pyc, and scss files. That is because none of those files should be served directly to the browser.

Some pure XBlocks, such as ORA2, use SCSS. That is fine, but it means ORA2 must compile that SCSS in CSS as part of the package publishing process. That way, when ORA2 is installed into edx-platform, the generated CSS is ready to be served without any further processing.

XModule-style [1] XBlocks, on the other hand, are treated differently. Several of these blocks have SCSS, but the SCSS is not compiled into CSS files in the XBlock resource_dirs. Instead, the SCSS is copied to common/lib/xmodule, using the xmodule_assets script defined here and declared here. Once copied, the SCSS is included into other LMS and CMS SCSS files; thus, the styles for these XBlocks are co-mingled with the styles for LMS and CMS.

We would like to get rid of the xmodule_assets script for a couple of reasons:

It makes the XBlock runtime in edx-platform harder to understand.
It makes it harder to deprecate and remove legacy LMS & CMS frontends, because the styles are tangled up with certain XBlocks' styles.
It is written in Python, which makes our static asset build process depend on Python, edx-platform code, and edx-platform Python requirements. This is very bad for build cache-ability. However, we could work around this problem in the short term by rewriting xmodule_assets in pure Bash:
- Rewrite XModule fragment copying in Bash #31611

For additional background on xmodule_assets, see the openedx-assets xmodule section of OpenCraft's discovery doc.

[1] By "XModule-style XBlocks", I mean: a set of older XBlock types declared in the ./xmodule/ directory of edx-platform, which rely on the xmodule_assets copying mechanims. The list is here. All other XBlocks types are considered "pure XBlocks".

Alternative

If we need an intermediate solution and/or if we think this issue is harder than it is worth, then we can instead do:

Rewrite XModule fragment copying in Bash #31611

Follow-up work

After the critical work in this epic is complete, further cleanup work could be done in:

Remove xmodule assets from the edx-platform build #32692

kdmccormick · 2023-02-23T22:24:16Z

FYI @Agrendalath , in case it helps with your discovery, I've updated this issue with what I've learned about xmodule_assets so far.

kdmccormick · 2023-02-28T20:27:33Z

Related discovery: https://docs.google.com/document/d/1FqsvXpvrzsi2Ekk9RttUpcT2Eg0NxenFmV52US_psFU/edit#heading=h.ogslfhqqejyh

kdmccormick · 2023-04-12T18:41:50Z

@andrey-canon could you leave a comment here so that GitHub will allow me to assign this issue to you?

andrey-canon · 2023-04-12T18:53:31Z

@andrey-canon could you leave a comment here so that GitHub will allow me to assign this issue to you?

no problem

kdmccormick · 2023-04-13T15:10:34Z

From #32018:

Kyle:
This is excellent. I am still testing it locally, but here are the only issues I found with the code itself.

Once this merges and the styles are decoupled, what do you think the next step should be?

Andrey: @kdmccormick, IMO this requires a style refactor, personally I don't like these module_styles_lines and descriptor module_styles_lines it'd be great if both of them depend on just one standard import or the same list of imports. In a bigger picture I don't have the answer right know, probably the next step should be identify and decouple more assets in order to do the loading process lighter or something more complex like try to migrate some xblocks to react applications since there are more independent from other assets.

I agree, although I think moving to react will be a longer-term challenge.

In the shorter term, I am hoping that we could do the XModule SCSS compilation without involving any Python code. On my branch, I have written a Bash script that compiles the LMS & CMS SCSS without invoking Python. Do you think we could do something similar for XModule SCSS?

andrey-canon · 2023-04-24T22:56:54Z

From #32018:

Kyle:
This is excellent. I am still testing it locally, but here are the only issues I found with the code itself.
Once this merges and the styles are decoupled, what do you think the next step should be?

Andrey: @kdmccormick, IMO this requires a style refactor, personally I don't like these module_styles_lines and descriptor module_styles_lines it'd be great if both of them depend on just one standard import or the same list of imports. In a bigger picture I don't have the answer right know, probably the next step should be identify and decouple more assets in order to do the loading process lighter or something more complex like try to migrate some xblocks to react applications since there are more independent from other assets.

I agree, although I think moving to react will be a longer-term challenge.

In the shorter term, I am hoping that we could do the XModule SCSS compilation without involving any Python code. On my branch, I have written a Bash script that compiles the LMS & CMS SCSS without invoking Python. Do you think we could do something similar for XModule SCSS?

@kdmccormick with this implemetation I think if you add the xmodule paths ( "common/static/xmodule/modules/scss" and "common/static/xmodule/descriptors/scss") to yours includes that will be enough

kdmccormick · 2023-04-25T04:33:54Z

Yes, I think that will let us compile the XModule SCSS into CSS without Python 👍🏻

However, we will still need static_assets.py in order to generate the input XModule SCSS files. Eventually, I would like get rid of that copying or rewrite it in Bash, so that all of openedx-assets build can be done without invoking Python.

This moves scss imports to a specific file where that is required instead of having a common XModule import list. This is part of openedx#31624

This moves XModule scss imports to the specific files where they are required instead of having a common XModule scss import list. This is part of #31624

This basically changes how the xmodule static files are generated and consumed in order to separate the Xblock styles from general style files. Addressing the issue openedx#31624

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 the 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 time when the XModule system was more heavily utilized, rather than just a legacy corner of the XBlock framework as it is today. 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 · 2023-08-11T18:43:54Z

This is done!

Follow-up work: #32692

kdmccormick mentioned this issue Jan 19, 2023

Rewrite XModule fragment copying in Bash #31611

Closed

1 task

kdmccormick changed the title ~~Remove special XModule asset processing~~ Remove special XModule asset handling Jan 19, 2023

This was referenced Jan 19, 2023

Extract XBlock assets from edx-platform #31622

Closed

Unify the descriptor/module system (Phase 2, xBlock conversion) openedx/platform-roadmap#12

Closed

kdmccormick mentioned this issue Feb 21, 2023

Build edx-platform assets without Python #31800

Open

kdmccormick mentioned this issue Feb 28, 2023

docs: add ADR: "Reimplement edx-platform static asset processing" #31790

Merged

This was referenced Mar 15, 2023

Tutor Adoption 4: Devstack Parity openedx/platform-roadmap#202

Closed

Catalog XModule assets #31932

Closed

Complete the MFE conversion process openedx/wg-frontend#156

Open

andrey-canon mentioned this issue Mar 31, 2023

Decouple XModule styles from LMS/Studio styles #32018

Merged

kdmccormick assigned kdmccormick and unassigned kdmccormick Apr 12, 2023

kdmccormick assigned andrey-canon Apr 12, 2023

kdmccormick mentioned this issue Apr 26, 2023

feat: move general imports to specific scss files #32121

Merged

andrey-canon added a commit to eduNEXT/edx-platform that referenced this issue Apr 26, 2023

feat: move general imports to specific scss files

89ca5e8

This moves scss imports to a specific file where that is required instead of having a common XModule import list. This is part of openedx#31624

kdmccormick pushed a commit that referenced this issue Apr 27, 2023

feat: move general imports to specific scss files (#32121)

f8e2363

This moves XModule scss imports to the specific files where they are required instead of having a common XModule scss import list. This is part of #31624

kdmccormick mentioned this issue May 23, 2023

[4/6] build: import XModule source SCSS directly rather than copying #32289

Merged

kdmccormick mentioned this issue May 23, 2023

[5/6] build: commit XModule SCSS entrypoints instead of generating them #32290

Merged

This was referenced May 23, 2023

[6/6] build: common/static/css/xmodule -> xmodule/static/css #32291

Merged

Stop dynamically generating XModule SCSS #32292

Closed

kdmccormick added epic Large unit of work, consisting of multiple tasks and removed epic Large unit of work, consisting of multiple tasks labels May 23, 2023

kdmccormick added the epic Large unit of work, consisting of multiple tasks label Jun 2, 2023

kdmccormick mentioned this issue Jun 15, 2023

Stop dynamically generating XModule JS #32481

Closed

This was referenced Jul 6, 2023

XModule Style Decoupling: Follow-up fixes & refactoring #32592

Merged

build: consolidate xmodule/js into xmodule/assets (WIP) #32691

Closed

Remove xmodule assets from the edx-platform build #32692

Open

kdmccormick added the code health Proactive technical investment via refactorings, removals, etc. label Jul 9, 2023

kdmccormick closed this as completed Aug 11, 2023

kdmccormick mentioned this issue May 1, 2024

[DEPR]: Paver #34467

Open

This was referenced Jul 19, 2024

[dupe] Complete the MFE conversion process for edx-platform #35145

Closed

Complete the MFE conversion process for edx-platform #31620

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove special XModule asset handling #31624

Remove special XModule asset handling #31624

kdmccormick commented Jan 19, 2023 •

edited

Loading

Tasks and PRs

kdmccormick commented Feb 23, 2023

kdmccormick commented Feb 28, 2023

kdmccormick commented Apr 12, 2023

andrey-canon commented Apr 12, 2023

kdmccormick commented Apr 13, 2023

andrey-canon commented Apr 24, 2023 •

edited

Loading

kdmccormick commented Apr 25, 2023

kdmccormick commented Aug 11, 2023

Remove special XModule asset handling #31624

Remove special XModule asset handling #31624

Comments

kdmccormick commented Jan 19, 2023 • edited Loading

Tasks and PRs

Background

Alternative

Follow-up work

kdmccormick commented Feb 23, 2023

kdmccormick commented Feb 28, 2023

kdmccormick commented Apr 12, 2023

andrey-canon commented Apr 12, 2023

kdmccormick commented Apr 13, 2023

andrey-canon commented Apr 24, 2023 • edited Loading

kdmccormick commented Apr 25, 2023

kdmccormick commented Aug 11, 2023

kdmccormick commented Jan 19, 2023 •

edited

Loading

andrey-canon commented Apr 24, 2023 •

edited

Loading