[1/6] refactor: define resource paths (not contents) on XModule classes by kdmccormick · Pull Request #32286 · openedx/openedx-platform

kdmccormick · 2023-05-23T14:53:06Z

Supporting information

This is a part of a series of PRs:

Stop dynamically generating XModule SCSS #32292

The previous series of PR is:

Decouple XModule styles from LMS/Studio styles (attempt 3) #32237

The next PR is:

[2/6] build: remove prefixes from XModule resource copies #32287

Description

For the XBlocks types that use legacy XModule-style assets (specifically, those that
inherit from HTMLSnippet), this is small refactor that brings them a bit closer to being like
standard XBlocks.

Given these class attributes:

class SomeXModuleLikeBlock(..., HTMLSnippet, ...):
    ...
    studio_view_css = { ... }
    preview_view_css = { ... }
    studio_view_js = { ... }
    preview_view_js = { ... }
    ...

we make it so their values are paths to the resources
rather than the actual content of the resources.
This is a no-op change, but it'll enable future XModule
asset refactorings which require us to operate on asset
paths rather than contents.

Other information

Reviewers: see #32286 (review)

Testing Instructions

Build the Tutor openedx image with this branch
Browse the demo course in the Studio outline editor
Browse the demo course in Learning MFE

Deadline

Medium-high urgency, as this is in the critical path to a long line of DevX improvement PRs.

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

Notes for reviewers 👀

kdmccormick · 2023-05-23T14:56:29Z

+            resource_filename(__name__, 'js/src/collapsible.js'),
        ],
-        'xmodule_js': resource_string(__name__, 'js/src/xmodule.js'),
+        'xmodule_js': resource_filename(__name__, 'js/src/xmodule.js'),


resource_string loaded the contents of the resource. We change it to resource_filename, which just returns the absolute path to the resource.

We make this change uniformly across all XModule-like XBlocks.

Non-blocking note: It looks like pkg_resources is deprecated and we should be using importlib.resources.path(package, resource). But annoyingly even that new way will be deprecated in Python 3.11 and replaced with a different interface.

It may be fine to leave as is for now, I've asked the setuptools community what the timeline for this deprecation is so we can plan accordingly.

kdmccormick · 2023-05-23T15:01:03Z

-            for idx, fragment in enumerate(class_css.get(filetype, [])):
+            for idx, fragment_path in enumerate(class_css.get(filetype, [])):
+                with open(fragment_path, 'rb') as fragment_file:
+                    fragment = fragment_file.read()


When processing the SCSS, we now must load the files rather than just looking at the contents.

This creates a very minor build-time inefficiency: any SCSS file that is used across multiple XModules will be loaded multiple times. On the other hand, it achieves a minor run-time optimization: these SCSS files are no longer loaded along with XModule classes.

The inefficiency will be gone soon, because follow-up refactorings aim to delete this processing step entirely.

kdmccormick · 2023-05-23T15:01:49Z

+        with open(module_js.get('xmodule_js'), 'rb') as xmodule_js_file:
+            xmodule_js_fragment = xmodule_js_file.read()
        # It will enforce 000 prefix for xmodule.js.
-        fragment_owners[(0, 'js', module_js.get('xmodule_js'))].append(getattr(class_, js_attribute + '_bundle_name')())
+        fragment_owners[(0, 'js', xmodule_js_fragment)].append(getattr(class_, js_attribute + '_bundle_name')())
        for filetype in ('coffee', 'js'):
-            for idx, fragment in enumerate(module_js.get(filetype, [])):
+            for idx, fragment_path in enumerate(module_js.get(filetype, [])):
+                with open(fragment_path, 'rb') as fragment_file:
+                    fragment = fragment_file.read()


(this is just the JS version of the SCSS change above; same considerations)

feanil

Seems reasonable to me, I tested locally by bringing up tutor and looking at the video block and seeing it rendered correctly in studio and the LMS.

feanil · 2023-05-24T14:50:31Z

+            resource_filename(__name__, 'js/src/collapsible.js'),
        ],
-        'xmodule_js': resource_string(__name__, 'js/src/xmodule.js'),
+        'xmodule_js': resource_filename(__name__, 'js/src/xmodule.js'),


Non-blocking note: It looks like pkg_resources is deprecated and we should be using importlib.resources.path(package, resource). But annoyingly even that new way will be deprecated in Python 3.11 and replaced with a different interface.

It may be fine to leave as is for now, I've asked the setuptools community what the timeline for this deprecation is so we can plan accordingly.

kdmccormick · 2023-05-24T15:24:49Z

Non-blocking note: It looks like pkg_resources is deprecated and we should be using importlib.resources.path(package, resource). But annoyingly even that new way will be deprecated in Python 3.11 and replaced with a different interface.

It may be fine to leave as is for now, I've asked the pypa/setuptools#3934 community what the timeline for this deprecation is so we can plan accordingly.

@feanil Good point, and thanks for opening that issue.

The direction that I hope we go for edx-platform XBlocks is to stop using pkg_resources and instead use the standard XBlock resource-loading framework (SharedBlockBase.open_local_resource, et al). Unfortunately, the XBlock framework itself uses pkg_resources. So:

open an issue in edx-platform to switch us over to standard XBlock resource management
open an issue in XBlock to switch over to importlib and/or the Python 3.11 replacement

kdmccormick · 2023-05-24T15:26:09Z

Sorry, I wasn't trying to ask you to open those issues, I meant to write:

So, I will:

open ...`

kdmccormick · 2023-05-25T16:53:49Z

Okay, it's a little more complicated than I thought. XBlock does not provide a resource abstraction, but xblock-utils does. If everyone just used xblock-utils' abstraction, then switching off of pkg_resources would be easy. Issues:

edx-pipeline-bot · 2023-05-25T18:43:10Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

edx-pipeline-bot · 2023-05-25T19:03:59Z

EdX Release Notice: This PR has been deployed to the production environment.

kdmccormick commented May 23, 2023

View reviewed changes

This was referenced May 23, 2023

[2/6] build: remove prefixes from XModule resource copies #32287

Merged

Remove special XModule asset handling #31624

Closed

kdmccormick changed the title ~~refactor: define resource paths (not contents) on XModule classes~~ [1] refactor: define resource paths (not contents) on XModule classes May 23, 2023

kdmccormick changed the title ~~[1] refactor: define resource paths (not contents) on XModule classes~~ [1/6] refactor: define resource paths (not contents) on XModule classes May 23, 2023

This was referenced May 23, 2023

Radically simplify XModule SCSS #32283

Closed

Stop dynamically generating XModule SCSS #32292

Closed

feanil approved these changes May 24, 2023

View reviewed changes

kdmccormick merged commit 0f847df into openedx:master May 25, 2023

kdmccormick deleted the kdmccormick/xmodule-resource-filename branch May 25, 2023 22:55

kenclary mentioned this pull request Jun 29, 2023

feat: basic get/post endpoint for v2 xblocks. TNL-10873 #32600

Merged

kenclary mentioned this pull request Aug 4, 2023

fix: fix mako runtime bug with studio_view. TNL-10912 #32912

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[1/6] refactor: define resource paths (not contents) on XModule classes - #32286

[1/6] refactor: define resource paths (not contents) on XModule classes#32286
kdmccormick merged 1 commit into
openedx:masterfrom
kdmccormick:kdmccormick/xmodule-resource-filename

kdmccormick commented May 23, 2023 •

edited

Loading

Uh oh!

kdmccormick left a comment

Uh oh!

kdmccormick May 23, 2023

Uh oh!

feanil May 24, 2023

Uh oh!

kdmccormick May 23, 2023 •

edited

Loading

Uh oh!

kdmccormick May 23, 2023

Uh oh!

feanil left a comment

Uh oh!

feanil May 24, 2023

Uh oh!

kdmccormick commented May 24, 2023

Uh oh!

kdmccormick commented May 24, 2023

Uh oh!

kdmccormick commented May 25, 2023

Uh oh!

edx-pipeline-bot commented May 25, 2023

Uh oh!

edx-pipeline-bot commented May 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

kdmccormick commented May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Supporting information

Description

Other information

Testing Instructions

Deadline

Uh oh!

kdmccormick left a comment

Choose a reason for hiding this comment

Uh oh!

kdmccormick May 23, 2023

Choose a reason for hiding this comment

Uh oh!

feanil May 24, 2023

Choose a reason for hiding this comment

Uh oh!

kdmccormick May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kdmccormick May 23, 2023

Choose a reason for hiding this comment

Uh oh!

feanil left a comment

Choose a reason for hiding this comment

Uh oh!

feanil May 24, 2023

Choose a reason for hiding this comment

Uh oh!

kdmccormick commented May 24, 2023

Uh oh!

kdmccormick commented May 24, 2023

Uh oh!

kdmccormick commented May 25, 2023

Uh oh!

edx-pipeline-bot commented May 25, 2023

Uh oh!

edx-pipeline-bot commented May 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kdmccormick commented May 23, 2023 •

edited

Loading

kdmccormick May 23, 2023 •

edited

Loading