Remove special XModule asset handling

kdmccormick commented 1 year ago

### Tasks and PRs
- [x] https://github.com/openedx/edx-platform/issues/31932
- [ ] https://github.com/openedx/edx-platform/pull/32121
- [ ] https://github.com/openedx/edx-platform/pull/32018
- [ ] https://github.com/openedx/edx-platform/pull/32188
- [ ] https://github.com/openedx/edx-platform/pull/32237
- [ ] https://github.com/openedx/edx-platform/issues/32292
- [ ] https://github.com/openedx/edx-platform/issues/32481

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:
- https://github.com/openedx/edx-platform/issues/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:

https://github.com/openedx/edx-platform/issues/31611

Follow-up work

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

https://github.com/openedx/edx-platform/issues/32692

kdmccormick commented 1 year ago

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 commented 1 year ago

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

andrey-canon commented 1 year ago

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

no problem

kdmccormick commented 1 year ago

From https://github.com/openedx/edx-platform/pull/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 commented 1 year ago

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 commented 1 year ago

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.

kdmccormick commented 1 year ago

With @andrey-canon 's asset decoupling complete, I can now see a viable path towards completely deleting xmodule/static_content.py. I'm hacking on it in this draft PR, which I'll probably break up into smaller PRs when it's ready.

kdmccormick commented 1 year ago

This is done!

Follow-up work: https://github.com/openedx/edx-platform/issues/32692

openedx / edx-platform