-
Notifications
You must be signed in to change notification settings - Fork 26
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
feat!: analytics and api deployment-specific runtime configuration #2412
Conversation
provider: "matomo"; | ||
endpoint: string; | ||
siteId: number; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new deployment config available (previously hardcoded into environment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, shall we add some example values to the debug deployment config, if you haven't done so already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I've just added a new siteId for storing debug analytics and will make a separate PR on the content repo to update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I can see how it makes sense to make the runtime deployment config values available through an injection token to make them immediately available.
Re Matomo – I've reached out to Lily who I believe is the person who actually uses the dashboard, to check the current usage and the possible advantages of setting different siteId
s at the deployment level. I'll add any updates here.
provider: "matomo"; | ||
endpoint: string; | ||
siteId: number; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, shall we add some example values to the debug deployment config, if you haven't done so already?
* Therefore all modules are defined and loaded as part of the core build process, | ||
* but it is still possible to override this file to create specific feature-optimised builds | ||
* | ||
* This is a feature marked for future implementation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I can see how listing the child modules here could work
I remember previously some of the plh deployments wanting specific analytics data (think plh-tz), but no idea if that is still the case or not. But yeah, once we know which deployments still want to collect analytics we can add site-specific configurations to the content repos instead |
PR Checklist
Description
Following on from #2409, make all modules and services read configuration variables from the runtime deployment config.
Specifically:
Breaking Changes (?)
Previously matomo analytics was initialized using variables hardcoded into environment.ts file, namely with hardcoded
siteId: 1
. I'm not sure if this was being overwritten on a per-deployment environment file level (I don't think we have per-deployment environment files?), so guessing all deployment analytics was going to the same matomo dashboard (possibly a bug?).Now the value can be configured from within the deployment config file (although default set to
siteId:1
to keep the same default as the hardcoded variables (so hopefully not a breaking change).Author Notes
Deployments can now be configured with analytics siteId and url (if using a different host than idems analytics). Deployments will require recompile and the
yarn start
command should be restarted to force copy of the deployment to the appReview Notes
yarn workflow deployment set
yarn start
, this should include the analytics configuration.Main thing to double check is that analytics and backend api working as expected (e.g. analytics and db syncing updates with correct deployment config)
Dev Notes
The major limitation with the previous implementation was that whilst the
APP_INITIALIZER
provider accepts async functions (i.e. wait for deployment service to load config json), no other angular providers accept async functions (https://github.com/angular/angular/issues/23279
). That means that any other code initalised within the app module would still init before deployment loaded - specifically the matomo module and API interceptor module, both of which require configuration from deployment.The workaround for this is to hoist one level higher, and import the json before the platform has even initialised (any modules or services imported). By doing this we can save the deployment config to an injection token (behaves like a global variable), that can then be accessed by any module or service during initialisation. There's still a little bit more work required to make sure module imports use correctly (see examples in code), but overall it removes some of the complexity from the APP_INITIALIZER function and should provide a more modular way to manage.
Separately I've refactored the analytics modules to import from a standalone file which also now injects the deployment config variables. The main reason for the separation is so that in the future we can make it easier to opt-out of entire modules by simply deleting files/folders and making minor text edits to
module.ts
files (angular doesn't support conditional modules due to the way the ahead-of-time compilation expects static imports to analyse - so if we don't want something included in the build then the import has to be removed). The full system for opt-in/opt-out features will be part of future work, this PR is more just to lay some groundwork and test various patterns.I've left in console logs for now to show loaded deployment config but assume these are probably best to remove for production (?)
For Follow-up
(moved to #2423)
Git Issues
Closes #
Screenshots/Videos
If useful, provide screenshot or capture to highlight main changes