feat!: make external scripts configurable per-app#227
Conversation
arbrandes
force-pushed
the
external-scripts
branch
from
b70d633 to
20f496c
Compare
MaxFrank13
left a comment
MaxFrank13
left a comment
There was a problem hiding this comment.
Seems clean! A long-awaited feature for sure. Just had one question
brian-smith-tcril
left a comment
brian-smith-tcril
left a comment
There was a problem hiding this comment.
Overall I think this makes a lot of sense. I do think there are a couple cases we should add documentation for though:
- Writing a little app to load a script directly in site config
- Likely a common use case, something we can support in Tutor MFE
- Writing a loader and adding it to specific apps
- The example in this PR is "the loader lives in an app, the app is always in the site" - having a "I just want to add this script on learner-dash" example would be nice
Another thing worth noting is that it seems like there are 2 ways to make sure a loader is always loaded: making an app for it and adding it to commonAppConfig. It'd be good to document why the app strategy is preferred over the commonAppConfig strategy (my understanding is that it's because if you put it in commonAppConfig you need to be more vigilant about having "don't run twice" guards in the loader itself)
|
We've decided to drop Google Analytics from here entirely. It's going to be a Tutor plugin instead. |
arbrandes
force-pushed
the
external-scripts
branch
5 times, most recently
from
2df91bb to
0b61eeb
Compare
I added a fuller (but still small) example to the ADR. I think it's good enough for now.
This was possible in MFE Land, but it doesn't translate here very well (or at all)... That is, unless we we wire up a complicated mechanism with an |
|
This is what the tutor plugin is going to look like: |
brian-smith-tcril
left a comment
brian-smith-tcril
left a comment
There was a problem hiding this comment.
LGTM!
I left a couple comments but nothing that should block merging this.
External scripts were previously hardcoded in initialize() with GoogleAnalyticsLoader as the default. This made them non-configurable and forced every site to bundle Google Analytics whether it was used or not. External scripts are now a per-app concern via the new externalScripts field on the App interface, following the same accumulation pattern as routes and slots. Each app's scripts receive that app's merged config (commonAppConfig + per-app config) via getAppConfig(). GoogleAnalyticsLoader is removed from frontend-base entirely. Operators who want it can author their own ExternalScriptLoader class and attach it to one of their apps. See ADR 0014 for full rationale. BREAKING CHANGE: GoogleAnalyticsLoader is no longer provided by frontend-base. Operators must supply their own external script loaders. Co-Authored-By: Claude <noreply@anthropic.com>
arbrandes
force-pushed
the
external-scripts
branch
from
0b61eeb to
c6732cf
Compare
|
🎉 This PR is included in version 1.0.0-alpha.28 🎉 The release is available on: Your semantic-release bot 📦🚀 |
4 participants
Description
External scripts were previously hardcoded in
initialize()withGoogleAnalyticsLoaderas the default. This made them non-configurable and forced every site to bundle Google Analytics whether it was used or not.This PR makes external scripts a per-app concern via a new
externalScriptsfield on theAppinterface, following the same accumulation pattern as routes and slots. Each app's scripts receive that app's merged config (commonAppConfig+ per-appconfig) viagetAppConfig().GoogleAnalyticsLoaderis removed from frontend-base entirely. Operators who want it can author their ownExternalScriptLoaderclass and attach it to one of their apps.See ADR 0014 for full rationale.
Demo
A working demonstration wiring a local
GoogleAnalyticsLoaderinto a site's configs lives at openedx/frontend-template-site#13.LLM usage notice
Built with assistance from Claude.