-
Notifications
You must be signed in to change notification settings - Fork 25
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: deployment build optimisation #2520
Open
chrismclarke
wants to merge
28
commits into
master
Choose a base branch
from
feat/deployment-build-optimisation
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Change how reports our output to include parent `data:[]` field. This improves ability to generate type definitions for reports (not manipulated prior to writing to disk)
chrismclarke
changed the title
Feat/deployment build optimisation
Feat!: deployment build optimisation
Nov 12, 2024
chrismclarke
added
the
breaking
Introduces breaking changes to how content is authored
label
Nov 12, 2024
chrismclarke
changed the title
Feat!: deployment build optimisation
Feat: deployment build optimisation
Nov 13, 2024
chrismclarke
removed
the
breaking
Introduces breaking changes to how content is authored
label
Nov 13, 2024
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Checklist
Future Follow-ups
Description
Add a new workflow
yarn workflow optimise_build
to remove unused code from deployment build.In it's first iteration there it performs the following optimisations:
By default the optimisation workflow is run as part of any app build, however will only change files if specific deployment configuration chooses to opt in.
Author Notes
Opt In
Deployments can opt in via a new
optimisation
config property. Currently defined asAdvanced Use Cases
There may be cases where some template components use other components within their core code, but where these implicit components are not referenced anywhere else and so removed from the build. E.g. the
task_card
component which uses thetask_progress_bar
component.If this happens the build will fail with an error such as
To fix this the optimisation can manually specify a list of components to include, e.g.
Alternatively we have started tracking a list of these implicit dependencies within the code itself, so if you notice any we can always update the list to save having to manually specify at the deployment level.
Local Diffs
Any deployment that has optimisation included will change the default
angular.json
file that is included in the main build. These changes shouldn't be committed to the main repo (not an issue when running build on github action, just need to remember to discard the changes if running build locally)Misc Changes
All generated reports now have a parent
data:
property used to improve consistency across reports for code checks (will show up a small diff when next syncing)Review Notes
yarn build
Additionally you can analyse the build bundle sizes by running the
yarn analyse
command (instead ofyarn build
)(see example screenshots)
If wanting to just run the optimisation step can manually call
yarn workflow optimise_build
(this is included in the build command)Tests can be run via
yarn workspace scripts test -t optimisers/components.spec.ts
Dev Notes
Review Support
There's quite a lot of changes here, many of which are just tidying the existing lists of components. If a bit confusing let me know and I can split the functional from non-functional changes and open as separate PRs.
All the code to handle optimisation processes should be within the optimisation scripts code folder. Tests are also included to cover the main methods and use-cases.
General Approach
The process of optimising components is a bit complex given that components can also depend on imported modules or modifications to angular.json (assets, styles or scripts). In addition, many components have implicit dependencies, where they import another component into the HTML templates which in turn may have its own requirements. As a result of this, the following approach has been implemented to try and solve:
Create a clear separation from common components used by templating to render outputs (e.g.
button
), base components defined in templates however removed during processing and non-rendered (e.g.set_field
), and a handful ofcore
components that are required by specific hardcoded functionality (e.g.text
which is used in not-found page redirect and other places)Develop a manifest schema that can be use to define component dependencies across module imports, angular json modifications and implicit components required
Create a new optimisation workflow and deployment configuration to enable build-time changes that modify core code, following analysis of which components are/aren't used in the deployment sheets and their corresponding manifests. The optimisations center around identifying imports no longer required and commenting out (allowing angular to drop from the build when tree-shaking). These changes are written to separate
.deployment
files (e.g.template.module.deployment.ts
) and overwritten during angular buildPerformance Gains
The main result of the current set of optimisations is to reduce the amount of code that will be bundled into the compiled
main.js
file. This will have the biggest performance gains on web, where the main.js file will have to be fully downloaded before the app can start to initialise. This file is typically around 1.1MB, when served over a host with gzip, and reduced by around 25-50% depending on what components the deployment is using (e.g. 700kb for plh_kids_kw, mostly saved from opting out of pdf and odk components).There are also some gains to android, however less significant as all files are included in the build and do not require internet to download. So the main improvement is just a slightly smaller overall bundles size when comparing the parsed bundle sizes before and after (3.85MB -> 2.44MB for plh_kids_kw). There will also be savings from bundled assets, e.g.
comp_pdf
bundles around 4.5MB of assets.There is unlikely to be much of a performance gain for general app speed/startup times as most of the blocking logic happens within service initialisation that isn't managed from components. This will be a future area of follow-up.
Limitations
There's noticeable bundle increase required to include dependencies for markdown (marked) and latex (katex), however as these dependencies are coded into pipes (instead of imported directly into components), it's not possible to remove (unless including duplicate dummy pipes). It would be good as a follow-up to refactor this to import directly into the component instead. Should also do the same for qr-code pipe.
The next larges bundle to remove would likely be intro.js, however as this is linked to actions we do not currently have a means to optimise. This could be a follow-up
It's not currently possible to opt out of components imported from the components package (e.g. plh components). As these cannot currently register additional 3rd party libs it is assumed they will be small in size so not a priority to remove from build. Likely a better solution in the future will be to move the additional component packages out of the main build entirely, and have some different means to import as requried per-deployment.
I had wanted to include the component manifest in the same
index.ts
file where the components were listed, however during to typescript interop issues this wasn't possible (scripts workspace is commonjs but src code is es2020, when importing from src also follows imports of individual components with@component
decorators which is also not supported). So using a separatemanifest.ts
file for now and could look to unify in the future.Additional Notes
The decision to comment out code and write to a separate file, instead of just removing from original, is to provide slightly easier comparison when inspecting builds and prevent build breaking when switching between deployments that do and do not use optimisations.
It wasn't possible to take the same approach to the
angular.json
file (e.g.angular.deployment.json
) as the angular cli does not support pointing to custom file paths. Whilst it would be possible to keep aangular.base.json
which is copied toangular.json
, this would mean removingangular.json
from git and no deployments working without the optimisation step carried out first (not a nice pattern).The option to provide deployment config with manual
implicit:[]
component definitions came before I added a more general component manifest approach, and so this can likely be removed in the future. I've kept in for now just in case we need to quickly fix emergent issues (but would still favour just updating the component manifests instead). It might also be useful in the future if we were to provide live-sync of sheets from an external source, so that an author could include components they no likely to be needed in the futureThe
display_group
component includes a number of implicit dependencies for highly specific use cases. It would be good to check if still in use and ideally refactor. For now there is a hardcoded hack to include these implicit dependencies. Reviewing other components it seems most are fairly pure (contain no other component references), however it might be good to get to a point where all either contain no other component references (beyond coretext
and a maybe a couple others), or are bundled together as a component pack to import. In the future I might just look at component prefixes, and if for example see any reference toparent_
import all components within that namespace (even if not all used).It's not currently possible to optimise ionic components, however it would be possible if we migrate to using standalone angular components. This could be considered as a follow-up (ionic does have auto code-gen to migrate: https://ionicframework.com/docs/angular/build-options#standalone)
Afterthought
There's part of me that thinks we might be going about this all backwards. Instead of starting with a full set of components and dependencies and removing what isn't required, it would probably make more sense to implement some sort of plugin system where components themselves specify the changes to be made to angular.json, package.json and any other files in order to implement (and then only run that code if included in templates). As of now if we add new components that require changes to angular.json then we need to manually update optimisations to include code to revert.
But this would be a larger feature to implement, so perhaps the optimisation system will be ok for now (at least until we have deployments that need to install packages that we haven't included). In these cases I expect short term we just add more packages as required to core (e.g. charts lib, payment processor etc.), and review in future best way to make more extensible.
So for now I've included a component
manifest
which is being used to handle knock-on effects of opting out of components, but in the future could also be used to generate the file updates required.Git Issues
Closes #
Screenshots/Videos
Before - compiled main.js for non-optimised deployments
After - plh_kids_kw main bundle smaller after dropping dependencies for odk, pdf and a number of other components
Example angular.json diff - removes comp-pdf assets not in use and includes file overrides
Example component index override - unused components are commented out and not included in build
Example module override - unused modules are commented out and not included in build