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.

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

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

Merged

Conversation

kdmccormick
Copy link
Member

@kdmccormick kdmccormick commented May 23, 2023

Supporting information

This is a part of a series of PRs:

The previous series of PR is:

The next PR is:

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
Copy link
Member Author

@kdmccormick kdmccormick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notes for reviewers 👀

'xmodule_js': resource_string(__name__, 'js/src/xmodule.js'),
'xmodule_js': resource_filename(__name__, 'js/src/xmodule.js'),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines -135 to +137
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()
Copy link
Member Author

@kdmccormick kdmccormick May 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +182 to +189
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()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@kdmccormick 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 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
Copy link
Contributor

@feanil feanil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

'xmodule_js': resource_string(__name__, 'js/src/xmodule.js'),
'xmodule_js': resource_filename(__name__, 'js/src/xmodule.js'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

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
Copy link
Member Author

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

So, I will:

  • open ...`

@kdmccormick
Copy link
Member Author

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:

@kdmccormick kdmccormick merged commit 0f847df into openedx:master May 25, 2023
@edx-pipeline-bot
Copy link
Contributor

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

@edx-pipeline-bot
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants