Skip to content
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

Add the ability to "inject" location into /fides.js bundles and cache responses for one hour #3272

Merged
merged 11 commits into from
May 10, 2023

Conversation

NevilleS
Copy link
Contributor

@NevilleS NevilleS commented May 10, 2023

Closes #3170, plus unticketed work to support injecting location 👍

Code Changes

  • Add cache-control headers to the /fides.js bundle to instruct browsers/CDNs to cache for ~1 hour
  • Add /fides.js?location= query param to inject a given location to the bundle
  • Add support for CloudFront location headers (e.g. CloudFront-Viewer-Country) to inject a given location to the bundle
  • Include the injected location in the ETag for caching /fides.js bundle
  • Add lots of tests

Steps to Confirm

  • Run all Cypress tests
  • Run nox -s "fides_env(test)", load the cookie house sample app, and confirm that /fides.js bundle is cached across page refreshes

Loom of some manual cache testing: https://www.loom.com/share/e840916e3c964b00b8d428a07492c9b4

Pre-Merge Checklist

Description Of Changes

This adds an (important!) optimization to the /fides.js route that will enable HTTP clients/CDNs/etc. to cache the results, so we don't unnecessarily hammer the Privacy Center with requests. This part was fairly easy to do, but I realized while doing this that we should bake in support for generating different bundles for different locations as early as possible- since those cache semantics are kinda hard to reason about...

So this PR also proactively adds the ability to request a fides.js bundle with an explicit location, e.g.: https://localhost:3000/fides.js?location=US-CA

☝️ This will parse the query param out, and add a location property to the config object passed into the bundled initialization of Fides.js, ie. window.Fides.init({ consent, location })

This'll should enable us to set up test harnesses to exercise different locations without resorting to a lot of VPN chicanery 😄

@NevilleS NevilleS requested a review from allisonking May 10, 2023 13:55
@NevilleS
Copy link
Contributor Author

I definitely didn't realize I had some mixing of fides.js builds from local branches! I think I fixed that now, but CI is still mysteriously cancelling/failing... hmm.

@cypress
Copy link

cypress bot commented May 10, 2023

Passing run #1870 ↗︎

0 4 0 0 Flakiness 0
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

Merge ba275da into c063274...
Project: fides Commit: 0337ac0924 ℹ️
Status: Passed Duration: 01:04 💡
Started: May 10, 2023 2:45 PM Ended: May 10, 2023 2:46 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

Copy link
Contributor

@allisonking allisonking left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is great, and thanks for all of the tests! 🎉

I left a few questions, but those are just to make sure I'm thinking about this the right way 👍

clients/privacy-center/common/location.ts Show resolved Hide resolved
clients/privacy-center/common/location.ts Outdated Show resolved Hide resolved
@NevilleS NevilleS merged commit d5769c1 into main May 10, 2023
@NevilleS NevilleS deleted the ns-fides-js-caching-3170 branch May 10, 2023 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimize Privacy Center /fides.js download with cache headers and a local public/assets/fides.js file
2 participants