feat!: make external scripts configurable per-app#227
feat!: make external scripts configurable per-app#227arbrandes wants to merge 1 commit intoopenedx:mainfrom
Conversation
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 moves to a new top-level contrib/ directory as a pre-built app that operators opt into via site.config. A wildcard exports map entry makes future contrib apps importable without changes to package.json. See ADR 0014 for full rationale. BREAKING CHANGE: GoogleAnalyticsLoader is no longer loaded by default. Operators must add the contrib app to their site.config to enable it. Co-Authored-By: Claude <noreply@anthropic.com>
arbrandes
force-pushed
the
external-scripts
branch
from
b70d633 to
20f496c
Compare
| without expanding the core runtime or the main export surface. | ||
|
|
||
| External script configuration (e.g. ``GOOGLE_ANALYTICS_4_ID``) lives in | ||
| ``commonAppConfig`` or per-app ``config``, not in the site config type. |
There was a problem hiding this comment.
lives in
commonAppConfigor per-appconfig, not in the site config type.
Isn't commonAppConfig in the site config?
There was a problem hiding this comment.
Yup! When you getAppConfig() the common config gets merged in.
There was a problem hiding this comment.
Oh, I see. This is meant to say not in the _top-level_ site config.
Nice catch!
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)
| loadScript(): void, | ||
| } | ||
|
|
||
| export type ExternalScriptLoaderClass = new (data: { config: AppConfig }) => ExternalScriptLoader; |
There was a problem hiding this comment.
I know we discussed switching to using config directly instead of data { config: } in the frontend-platform PR. That one landed with the data object.
At this point I think I'm fine either way, but it'd be good to be consistent.
| @@ -1,5 +1,6 @@ | |||
| module.exports = { | |||
| projects: [ | |||
| 'contrib', | |||
There was a problem hiding this comment.
If this can live in a different repo instead of here that'd be awesome! Will follow up after the standup discussion.
| Operators must explicitly opt in to Google Analytics by adding the contrib app | ||
| to their ``apps`` array in ``site.config``. Sites that do not use Google | ||
| Analytics no longer load the script by default. |
There was a problem hiding this comment.
We might want to include the GA app in the template site used by Tutor MFE until we fully DEPR GA.
| commonAppConfig: { | ||
| GOOGLE_ANALYTICS_4_ID: 'G-XXXXXXXXXX', | ||
| }, |
There was a problem hiding this comment.
With the loader living it its own app this wouldn't need to be in commonAppConfig would it? It could just be in the GA app config?
| apps.forEach(app => { | ||
| const config = getAppConfig(app.appId) ?? {}; | ||
| (app.externalScripts ?? []).forEach(ExternalScript => { | ||
| const script = new ExternalScript({ config }); |
There was a problem hiding this comment.
When discussing the frontend-platform PR the topic of potentially not having loadExternalScripts create new instances came up.
I'm wondering if we need the loaders to be classes at all, or if they can just be functions that take in config as a param.
I'm not opposed to sticking to classes (it seems to have been working for the GA loader for a long time), but I'd like to think through ways to possibly simplify this.
|
We've decided to drop Google Analytics from here entirely. It's going to be a Tutor plugin instead. |
3 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().GoogleAnalyticsLoadermoves to a new top-levelcontrib/directory as a pre-built app that operators opt into viasite.config. A wildcard exports map entry ("./contrib/*") makes future contrib apps automatically importable without changes topackage.json.See ADR 0014 for full rationale.
Test/Demonstration
Run the following of template-site against this PR (by the usual expedient of checking out this version of frontend-base into
packages/frontend-base):openedx/frontend-template-site#13
If you look in the Network tab, you should see a request go out to Google's servers. (In particular, to
https://www.googletagmanager.com/gtag/js?id=G-TEST123.)LLM usage notice
Built with assistance from Claude.