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

Allows CDN to cache empty experiences responses from fides.js API #4113

Merged
merged 6 commits into from
Sep 19, 2023

Conversation

eastandwestwind
Copy link
Contributor

@eastandwestwind eastandwestwind commented Sep 18, 2023

Closes #4108

Description Of Changes

Updates experience API to return empty obj instead of undefined when no experience was found for a specific location. This allows cache to work properly under this scenario, meaning that requests to the server will decrease overall.

Code Changes

  • Return empty obj for fides.js experience API when no experience was found
  • Update e2e tests

Steps to Confirm

  • Use a prefetch enabled privacy center and fides.js via the CDN. Ensure that debug mode is on.
  • Ensure the location you are in does not have a valid privacy notice
  • Go to the URL where fides.js is installed (i.e. Cookie House) and inspect the console
  • View the line that starts with Got experience response from Fides API, returning: and see that {} is returned instead of being undefined or null.
Screenshot 2023-09-18 at 5 02 57 PM

Pre-Merge Checklist

@eastandwestwind eastandwestwind changed the title Experience API should return empty obj instead of undefined when no experience was found for location Fides js should allow CDN to cache experience resp for locations with no experience Sep 18, 2023
@eastandwestwind eastandwestwind changed the title Fides js should allow CDN to cache experience resp for locations with no experience Allows CDN to cache empty experiences responses from fides.js API Sep 18, 2023
@cypress
Copy link

cypress bot commented Sep 18, 2023

Passing run #4223 ↗︎

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 b211b53 into b770602...
Project: fides Commit: 214046a081 ℹ️
Status: Passed Duration: 01:09 💡
Started: Sep 19, 2023 4:22 PM Ended: Sep 19, 2023 4:23 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.

ah this is a tricky problem!!

can we try to make the types stricter? I think once we make it stricter, some other pieces will fall into place, and we might not need some of the helper methods 🤞 of course, TS can often be tricky, so maybe my theory won't pan out, but I think it's worth trying, and will be less likely to bite us in the future

clients/fides-js/src/fides.ts Show resolved Hide resolved
clients/fides-js/src/fides.ts Outdated Show resolved Hide resolved
clients/fides-js/src/fides.ts Outdated Show resolved Hide resolved
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.

🎉

clients/fides-js/src/lib/consent-utils.ts Show resolved Hide resolved
clients/fides-js/src/lib/consent-utils.ts Outdated Show resolved Hide resolved
@eastandwestwind eastandwestwind merged commit d5c5293 into main Sep 19, 2023
10 checks passed
@eastandwestwind eastandwestwind deleted the 4108-cache-empty-experiences branch September 19, 2023 16:34
eastandwestwind added a commit that referenced this pull request Sep 19, 2023
SteveDMurphy pushed a commit that referenced this pull request Sep 22, 2023
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.

No caching/prefetch available for regions without privacy notice(s)
2 participants