-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Unmask types with explicit mode: "unmask"
and default to leave the type alone
#12252
Conversation
✅ Docs preview readyThe preview is ready to be viewed. View the preview File Changes 0 new, 1 changed, 0 removed
Build ID: d845afbec06eb517728f4ae5 URL: https://www.apollographql.com/docs/deploy-preview/d845afbec06eb517728f4ae5 |
🦋 Changeset detectedLatest commit: f7d8e2b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
commit: |
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
size-limit report 📦
|
@phryneas this PR is good to review code-wise, but I'm working on the docs updates along with this, hence the draft status. Feel free to add suggestions though for the code itself 🙂 |
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 looks good to me code-wise. Once you are happy with the word-smithing, please feel free to get this out
src/masking/types.ts
Outdated
@@ -44,8 +44,11 @@ export type MaybeMasked<TData> = | |||
// prevent "Type instantiation is excessively deep and possibly infinite." | |||
true extends IsAny<TData> ? TData | |||
: TData extends { __masked?: true } ? Prettify<RemoveMaskedMarker<TData>> | |||
: DataMasking extends { enabled: true } ? TData | |||
: true extends ContainsFragmentsRefs<TData> ? Unmasked<TData> | |||
: DataMasking extends { mode: "preserveTypes" } ? TData |
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.
I opted to make this explicit even though it is the default behavior without this line, that way we have a reference to it in code in case someone searches this codebase (see the docs changes for how this is conveyed in docs).
|
||
To turn on masked types for your whole application at once, modify the `DataMasking` exported type from `@apollo/client` using TypeScript's [declaration merging](https://www.typescriptlang.org/docs/handbook/declaration-merging.html) ability. | ||
<MinVersion version="3.12.5"> | ||
#### Setting a types mode for masked types |
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 the biggest change in the docs for this PR. With the move to mode
instead of enabled
, it didn't make much sense to talk about this as a "opt into masked types", since really all that was doing was preserving the type and preventing it from being unwrapped. I've oriented this now around the modes and what they do.
I've provided a migration path for 3.12.5 for those that want/need it in the PR description. Is that something we should include in the docs as well, or should we point to the PR for these instructions instead?
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.
In the past, I've advocated for having this type of information in the docs so that users don't have to look in two places to learn how to do something. In OSS-world though, it might be reasonable to point folks to a PR description though.
If folks are likely to be looking at just the docs and not also the PR (because they're curious about the how the changes were implemented), my (weakly-held) opinion is to cut and paste what you've written in the PR description into to the docs.
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.
I like the changes you made here 👍
@@ -1419,9 +1423,9 @@ new ApolloClient({ | |||
|
|||
> Enabling data masking early in the adoption process makes it much easier to adopt for newly added queries and fragments since masking becomes the default behavior. Ideally data masking is enabled in the same pull request as the `@unmask` changes to ensure that no new queries and fragments are introduced to the codebase without the `@unmask` modifications applied. | |||
|
|||
#### 3. Generate and opt in to use masked types |
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.
There really is no "opt in" anymore after this change so I tried to remove that language everywhere I could.
@Meschreiber the docs changes should be good to review now, barring any significant feedback that the switch to |
I switched the approach and would prefer a new review
!docs preview |
src/masking/types.ts
Outdated
@@ -44,7 +44,8 @@ export type MaybeMasked<TData> = | |||
// prevent "Type instantiation is excessively deep and possibly infinite." |
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.
I would suggest moving the DataMasking
tests up:
export type MaybeMasked<TData> =
DataMasking extends { mode: "unmask" } ?
// distribute TData - in case of a union, do the next steps for each member
TData extends any ?
// prevent "Type instantiation is excessively deep and possibly infinite."
true extends IsAny<TData> ? TData
: TData extends { __masked?: true } ? Prettify<RemoveMaskedMarker<TData>>
: true extends ContainsFragmentsRefs<TData> ? Unmasked<TData>
: TData
: never
: DataMasking extends { mode: "preserveTypes" } ? TData
: TData;
This way, in cases where mode
is not "unmask"
, the types won't be touched at all.
While that might seem counterintuitive, the benefit is that TData
itself will not need to be inspected at all, which enables cases where TData
is unknown at the time of invoking MaybeMasked
.
Here is a new test that shows the problem:
test("MaybeMasked can be called with a generic if `mode` is not set to `unmask`", (prefix) => {
function withGenericResult<T extends { [key: string]: string }>(
arg: TypedDocumentNode<T, {}>
) {
bench(prefix + "Result generic - instantiations", () => {
const maybeMasked: MaybeMasked<typeof arg> = arg;
return maybeMasked;
}).types();
bench(prefix + "Result generic - functionality", () => {
const maybeMasked: MaybeMasked<typeof arg> = arg;
expectTypeOf(maybeMasked).toEqualTypeOf(arg);
});
}
function withGenericDocument<T extends TypedDocumentNode>(arg: T) {
bench(prefix + "Result generic - instantiations", () => {
const maybeMasked: MaybeMasked<T> = arg;
return maybeMasked;
}).types();
bench(prefix + "Result generic - functionality", () => {
const maybeMasked: MaybeMasked<T> = arg;
// cannot use unresolved generic with `expectTypeOf` here so we just try an assignment the other way round
const test: T = maybeMasked;
return test;
});
}
withGenericResult({} as any);
withGenericDocument({} as any);
});
The drawback is that the Masked
marker would not be removed, but if you don't set { mode: "unmask" }
I don't think Masked
has any use case, right?
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.
The drawback is that the
Masked
marker would not be removed, but if you don't set { mode: "unmask" } I don't thinkMasked
has any use case, right?
Thats correct! In fact, I added a note to the changes in these docs that mention that: https://github.com/apollographql/apollo-client/pull/12252/files#diff-bc07ed3996b3e2e04d5117c660c968e40ca4802d650121ec40e236308f073cdbR1213-R1217
##### Using per-operation masked types
<Note>
This is only useful if you have changed the types mode to `unmask`. Using this with `preserveTypes` has no effect.
</Note>
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.
Just a couple of suggestions on my end! Will commit one change directly momentarily.
|
||
To turn on masked types for your whole application at once, modify the `DataMasking` exported type from `@apollo/client` using TypeScript's [declaration merging](https://www.typescriptlang.org/docs/handbook/declaration-merging.html) ability. | ||
<MinVersion version="3.12.5"> | ||
#### Setting a types mode for masked types |
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.
In the past, I've advocated for having this type of information in the docs so that users don't have to look in two places to learn how to do something. In OSS-world though, it might be reasonable to point folks to a PR description though.
If folks are likely to be looking at just the docs and not also the PR (because they're curious about the how the changes were implemented), my (weakly-held) opinion is to cut and paste what you've written in the PR description into to the docs.
Co-authored-by: Maria Elisabeth Schreiber <maria.schreiber@apollographql.com>
Co-authored-by: Maria Elisabeth Schreiber <maria.schreiber@apollographql.com>
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.
All good from the docs perspective!
enabled: false
and default to leave the type alonemode: "unmask"
and default to leave the type alone
Fixes #12248
Fixes #12251
After enough feedback from users upgrading to 3.12.x, we feel the default behavior of applying
Unmasked
withMaybeMasked
is the wrong default since this does type modification. That change has made it difficult for those currently using GraphQL Codegen's fragment masking feature and has introduced regressions in TypeScript compilation performance (#12248) simply from upgrading from an older version of the client.This PR changes the default behavior of
MaybeMasked
to do nothing with the types unless otherwise specified. Theenabled
flag has been deprecated in favor of a newmode
property which better describes the two different behaviors used for TypeScript:preserveTypes
This is the new default and will simply leave
TData
alone with no type modification. Since this is the new default, users don't need to set this to get this behavior.unmask
This will unwrap all operation types by default into their unmasked form. This is the same as the 3.12.4 or below default. Users will now need to explicitly opt into using the unmasked types
Setting the mode
To set the mode, you use the same declaration merging syntax as before:
Migrating from <= v3.12.4
If you've opted into masked types by setting the
enabled
property totrue
, you can remove this configuration entirely:If you prefer to specify the behavior explicitly, switch from
enabled: true
, tomode: "preserveTypes"
:If you rely on the default behavior in 3.12.4 or below and would like to continue to use unmasked types by default, set the
mode
tounmask
: