Skip to content

Conversation

@kdmccormick
Copy link
Member

We update the static assets plan based on three major things we've learned:

  1. You can stick any old script under npm run via package.json, and apparently that's the standard thing to do in frontend packages. So, forget scripts/build-assets.sh, let's do npm run build!
  2. Removing the special XModule asset processing was necessary in order to achieve most of the optimization goals of this ADR, so we went ahead and did it. We can remove the xmodule_assets step entirely now.
  3. Upgrading from libsass-python to node-sass/dart-sass is hard any nobody has time to do it right now. Even sidegrading to a another old non-Python version of libsass proved to be difficult when I tried. So, we are stuck with libsass-python for now, which means we are stuck with writing the compile-sass script in Python. This is fine, as we can still get most of the benefits we need as long as we segment out the requirements for compile-sass from the rest of edx-platform.

Links:

@kdmccormick kdmccormick force-pushed the kdmccormick/new-assets-adr branch 3 times, most recently from e647836 to 9561a2f Compare July 20, 2023 20:53
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.

Extra notes for reviewers.

Comment on lines +28 to +29
Copy link
Member Author

@kdmccormick kdmccormick Jul 20, 2023

Choose a reason for hiding this comment

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

Timestamping this since the line between XModule and XBlock has blurred since writing. Still, the table makes sense from the perspective of this ADR.

Comment on lines +133 to +137
Copy link
Member Author

Choose a reason for hiding this comment

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

As I moved towards npm run, I moved away from the structure of openedx-assets.

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 note seemed irrelevant now that npm run watch is an entirely separate command.

Comment on lines -231 to -239
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 whole OPENEDX_BUILDS_ASSETS_OPTS idea doesn't play well with the simpler npm run command structure, so I removed it in favor of simple environment variables (below).

Comment on lines -341 to -344
Copy link
Member Author

Choose a reason for hiding this comment

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

We aren't entirely removing Python any more, so I removed this section, but I folded its arguments into the rejection of "Improve existing system" since they are largely the same.

@kdmccormick kdmccormick marked this pull request as ready for review July 20, 2023 20:57
@kdmccormick kdmccormick requested a review from feanil July 20, 2023 20:57
@kdmccormick kdmccormick force-pushed the kdmccormick/new-assets-adr branch from 9561a2f to 81c7c10 Compare July 20, 2023 20:59
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.

One non-blocking comment but generally looks like a good improvement which leads us closer to npm and node for front-end things and python/django for backend/historic things.

I like that we don't have to have people know about a new interface for invoking commands.

Copy link
Contributor

Choose a reason for hiding this comment

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

Love this, though I know it may feel a bit duplicative to have this info in both an environment variable and in the LMS settings. In the future we could allow the user to override the setting based on a value in the environment as well so I'm not too worried about it for now.

There is an implied "We expect Django and Node to put assets in the same place for things to render properly." Is this written down somewhere or should we document it?

(Non-blocking thought.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed all around. The specifics of things like the exact env var semantics are still in flux (hence this PR :) so I'm hoping that this ADR will tide over anyone who wants to experiment with the new assets system. Once I'm closer to removing Paver, though, I'll make sure that these sort of settings are either de-duped or documented.

@kdmccormick kdmccormick force-pushed the kdmccormick/new-assets-adr branch from 81c7c10 to 7f354ca Compare July 21, 2023 02:38
@kdmccormick kdmccormick merged commit 4f393a1 into openedx:master Jul 21, 2023
@edx-pipeline-bot
Copy link
Contributor

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

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

@kdmccormick kdmccormick deleted the kdmccormick/new-assets-adr branch July 27, 2023 14:48
@kdmccormick kdmccormick mentioned this pull request May 1, 2024
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