-
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 TCF bundle with just-in-time GVL translations #5074
Optimize TCF bundle with just-in-time GVL translations #5074
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
Passing run #8974 ↗︎
Details:
Review all test suite changes for PR #5074 ↗︎ |
f901d27
to
d22bfb1
Compare
gvlVendors: activeChunk?.filter((v) => v.isGvl), | ||
otherVendors: activeChunk?.filter((v) => !v.isGvl), | ||
}), | ||
[activeChunk] | ||
); | ||
|
||
if (!activeChunk) { | ||
return null; | ||
} | ||
|
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.
handling (unlikely) scenario of having 0 vendors configured for a given experience
469e8c6
to
efc0b1b
Compare
"gvlSpecificationVersion": 3, | ||
"vendorListVersion": 40, | ||
"tcfPolicyVersion": 4, |
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.
resolves a Typescript complaint that these were missing
Reviewing now... |
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.
This is on the right track after testing locally. Let's re-test once BE is in place
667139b
to
8e1ebee
Compare
Just a note that after https://github.com/ethyca/fidesplus/pull/1510 is merged, lets remove the |
ac24959
to
04673ff
Compare
@@ -483,49 +463,6 @@ describe("i18n-utils", () => { | |||
expect(getRecordCounts(loadedMessagesEn)).toMatchObject(expectedCounts); | |||
expect(getRecordCounts(loadedMessagesEs)).toMatchObject(expectedCounts); | |||
}); | |||
|
|||
it("handles a mismatch between the experience_config and gvl_translations APIs by returning only locales available in both", () => { |
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.
this is all handled server-side now
@@ -224,13 +224,7 @@ export function loadMessagesFromFiles(i18n: I18n): Locale[] { | |||
|
|||
/** | |||
* Parse the provided PrivacyExperience object and load all translated strings | |||
* into the message catalog. Extracts translations from two sources: | |||
* 1) experience.experience_config | |||
* 2) experience.gvl_translations |
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.
#2 now happens as a separate process during initOverlay
expect(data.signalStatus).to.eql("ready"); | ||
expect(data.applicableSections).to.eql([-1]); |
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.
developer's local Admin UI config could make this behave differently because we're testing from /fides-js-demo.html
instead of /fides-js-components-demo.html
in this particular test. Removing these 2 extra, flaky checks in favor of the original intent of the test, which is to check that GPP loads and can be pinged. (The other gets tested elsewhere)
data_sales: false, | ||
tracking: false, | ||
analytics: true, |
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.
Again, because we're testing with /fides-js-demo.html
here instead of /fides-js-components-demo.html
, these fields can be different per developer's environment and don't relate to the actual intent of this particular test.
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.
Looking good so far, just some notes to remove the gvl_translations
obj later, including the gvl_translations: Optional[Dict] = {}
we define on the SQLAlchemy obj
mimic future state more closely by extrapolating those available_locales in the FE as well as passing those back to the /api/v1/plus/gvl/translations endpoint even though the filter isn't in place yet
f7fa793
to
384a8ae
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5074 +/- ##
==========================================
+ Coverage 86.47% 86.48% +0.01%
==========================================
Files 357 357
Lines 22271 22270 -1
Branches 2944 2944
==========================================
+ Hits 19258 19260 +2
+ Misses 2498 2495 -3
Partials 515 515 ☔ View full report in Codecov by Sentry. |
Passing run #8978 ↗︎
Details:
Review all test suite changes for PR #5074 ↗︎ |
Closes PROD-2281
Description Of Changes
In an effort to reduce initial payload size of TCF version of FidesJS, we now ignoring GVL translations in the experience until just as we're rendering the UI.
This is being tightened up by calling the dedicated endpoint for GVL translations.
Code Changes
gvl_translations
from the initialfides.js
payload.gvl_translations
from/api/v1/plus/gvl/translations
to use just as the UI is rendering.Steps to Confirm
/fides-js-demo.html?geolocation=fr-id
api/v1/privacy-experience
Calling Fides GET GVL translations API...
andRecieved GVL languages response from Fides API
Fides has been initialized!
message should appear prior to theRecieved GVL languages response from Fides API
message in the console. We don't want to delay the overall initialization for this.Pre-Merge Checklist
CHANGELOG.md