-
Notifications
You must be signed in to change notification settings - Fork 72
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
Optimize FidesJS bundle size by 40% by only loading TCF static strings when needed #4711
Optimize FidesJS bundle size by 40% by only loading TCF static strings when needed #4711
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
…mize-fides-build-exclude-tcf-strings
Passing run #6674 ↗︎
Details:
Review all test suite changes for PR #4711 ↗︎ |
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.
Amazing! 💯
@@ -11,8 +11,8 @@ import commonjs from "@rollup/plugin-commonjs"; | |||
|
|||
const NAME = "fides"; | |||
const IS_DEV = process.env.NODE_ENV === "development"; | |||
const GZIP_SIZE_ERROR_KB = 65; // fail build if bundle size exceeds this | |||
const GZIP_SIZE_WARN_KB = 55; // log a warning if bundle size exceeds this | |||
const GZIP_SIZE_ERROR_KB = 45; // fail build if bundle size exceeds this |
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.
🎉
* having to do any particularly difficult tree-shaking! | ||
*/ | ||
import type { I18n, Locale } from "../../i18n"; | ||
import { STATIC_MESSAGES_TCF } from "./locales"; |
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.
So basically, you've moved all the TCF-specific locales into the tcf
-specific folder so that those don't get picked up and loaded into the message catalog by the non-TCF build. Then here, when we know we're working with the TCF-specific bundle, we load them in dynamically. Is that right?
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.
Yup, pretty much! The only difference is that it's not really "dynamically" - it's statically true that this entire file (tcf-i18n-utils
) is only reachable from the fides-tcf.ts
entrypoint, so that makes it really obvious to rollup that it's not needed by the regular fides.ts
entrypoint.
* TCF-specific i18n utility functions. These are isolated to a separate file so | ||
* that Rollup can easily remove this code from the base fides.js bundle without | ||
* having to do any particularly difficult tree-shaking! |
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.
OK so we can't share the non-tcf loadMessagesFromFiles
method from i18n-utils.ts
because of difficulties with Rollup, I remember running into this before, with trying to share methods between non-tcf and tcf
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.
We could technically call any of those methods from this file without any issue; we just need to be careful not to go the other direction and have the tcf/i18n/**
files get imported by anything in the base package 👍
…mize-fides-build-exclude-tcf-strings
Closes PROD-1796
Description Of Changes
This optimizes the fides.js bundle size by splitting the static
i18n
message catalogs into two chunks:lib/i18n/locales/**/messages.json
lib/tcf/i18n/locales/**/messages-tcf.json
We then only load the TCF-specific messages during the initialization of the TcfOverlay, which is only called by the fides-tcf.js entrypoint. This allows rollup to correctly exclude all the TCF-specific JSON files from the default fides.js bundle and reduces the fides.js bundle size by ~22KB, an excellent 40% reduction!
Code Changes
loadTcfMessagesFromFiles
helper inlib/tcf/i18n
to load TCF messagesrollup-plugin-visualizer
to dev dependencies to visualize bundle:(shows that the remaining
locales
data files are still about 15% of the overall bundle size, pre-minification/gzip)Steps to Confirm
consent-i18n
E2E tests and confirm that all existing translations work as expected!Pre-Merge Checklist
CHANGELOG.md