Rework data masking types - #12837
Conversation
…d. Rename MaybeMasked implementation to Mask
🦋 Changeset detectedLatest commit: 9a7c492 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Docs preview readyThe preview is ready to be viewed. View the preview File Changes 26 new, 66 changed, 5 removedBuild ID: 4dc20d93b56070a09151783c URL: https://www.apollographql.com/docs/deploy-preview/4dc20d93b56070a09151783c |
commit: |
size-limit report 📦
|
| DefaultImplementation, | ||
| TData | ||
| >; | ||
| export type MaybeMasked<TData> = |
There was a problem hiding this comment.
I opted to leave MaybeMasked here since we still can't determine at any given time when to use the masked type or to unwrap the type with unmask.
If we completely removed MaybeMasked, we'd force all formats in the future to output the masking types since the rest of Apollo Client would need to remove MaybeMasked<TData> in favor of plain TData. This wouldn't work super great with the preset output as you suggest since we have no additional wrapping type to know how to pull out the masked/unmasked type.
As you can see, what I did here instead is remove MaybeMasked as an HKT type in favor of a new overridable IsMaskingEnabled type for determing if masking is enabled. In the case of GraphQLCodegen, we can assume it always generates the masked type. In the case of our preset in the future, we can update this to check for the presence of that updated format and unwrap it with the Mask type. This should cover all cases, whether or not the masked type is generated by default or is wrapped in a different object that nests the masks/unmasked variants.
ebe5325 to
6e74f21
Compare
There was a problem hiding this comment.
Look at this from a zoomed out view, does this change make any sense? It might be worth just leaving the names alone to avoid confusion. Would love your thoughts.
There was a problem hiding this comment.
Made an executive decision to leave as-is. I saw no good reason to change as it felt like a change for the sake of change.
| export interface Implementation { | ||
| Masked: HKTImplementation.Masked; | ||
| MaskedDocumentNode: HKTImplementation.MaskedDocumentNode; | ||
| export interface TypeOverrides { |
There was a problem hiding this comment.
Renamed to TypeOverrides to make this more obvious what they are used for. This reads nicely when used in end-user code:
declare module "@apollo/client" {
export interface TypeOverrides extends GraphQLCodegenDataMasking.TypeOverrides {}
}
| } | ||
| } | ||
|
|
||
| { |
There was a problem hiding this comment.
With the Masked type removed, these sections became copies of the block right before this so I removed it to avoid an unnecessary duplicate.
phryneas
left a comment
phryneas
left a comment
There was a problem hiding this comment.
Generally approved, assuming a few documentation additions.
|
|
||
| 3. [Enable data masking](#enabling-data-masking) in Apollo Client. | ||
|
|
||
| #### Setting a types mode for masked types |
There was a problem hiding this comment.
This document now needs some information on how to opt into the data masking types for codegen.
import { GraphQLCodegenDataMasking } from "@apollo/client/masking";
declare module "@apollo/client" {
export interface TypeOverrides extends GraphQLCodegenDataMasking.TypeOverrides {}
}There was a problem hiding this comment.
Good call. I'll make sure that goes in #12835
| type DefaultImplementation = GraphQLCodegenDataMasking.Implementation; | ||
| import type { PreserveTypes } from "./PreserveTypes.js"; | ||
|
|
||
| export interface DataMasking {} |
There was a problem hiding this comment.
Should we also remove this interface?
|
I'm going to apply one change (the suggested inlining in Everything else can easily be a follow-up, but it's probably good to get this out into a RC sooner than later. |
2 participants
While working on #12835 we found some opportunity for improvement on the data masking types.
MaskedandMaskedDocumentNodetypes have been removed as these no longer have any effect with the default type preservation