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

Enable data masking in cache instead of client #12019

Closed
wants to merge 13 commits into from

Conversation

jerelmiller
Copy link
Member

@jerelmiller jerelmiller commented Aug 23, 2024

Currently you enable dataMasking by setting the option in the ApolloClient constructor. While this has worked well throughout the development of this feature, this approach does not work very well when using the watchFragment API to mask fragment data returned from that method. That method needs to check whether to mask the raw cache data or leave as-is and it has no way of doing that with the current setup.

We approached this earlier in #11891 which used the ApolloClient constructor to inform the cache when the option was enabled. This however was clunky and thus moved in #11913 which was a more natural API. Unfortunately #11913 now again makes it difficult to determine in the cache whether masking is enabled in the cache. For the cache to determine whether masking is enabled, we'd need to revert back to a solution like #11891. At this point though this seems like a really roundabout way to avoid configuring this option in the cache itself. Instead, I'm proposing we move this configuration to the cache.

This approach will still work for any other cache implementation. Those cache implementations can implement the necessary API to use this feature and provide its own interface for enabling masking for its end users.

Copy link

changeset-bot bot commented Aug 23, 2024

⚠️ No Changeset found

Latest commit: 7c2b71c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@jerelmiller jerelmiller added the 🎭 data-masking Issues/PRs related to data masking label Aug 23, 2024
@jerelmiller jerelmiller requested a review from phryneas August 23, 2024 00:36
@jerelmiller jerelmiller force-pushed the jerel/enable-masking-in-cache-config branch from b8fb05e to 311eb6d Compare August 23, 2024 00:38
@jerelmiller
Copy link
Member Author

Discussed with @phryneas and we think that moving masking into the client.* APIs (i.e. client.readFragment(...), client.watchFragment(...), etc). should be the way to go for now. This means that if you use the same underlying cache APIs directly (cache.readFragment(...), etc), the data returned here will not be masked.

This is a point we will hope to gather feedback on during an experimental release to see if this ends up too confusing or not. As such, we'll keep the config value where it is.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🎭 data-masking Issues/PRs related to data masking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant