Improve TypeScript type-safety of `cache.modify` #10895

Gelio · 2023-05-19T11:31:27Z

This PR contains 2 changes that aim to improve the type safety of fields in cache.modify.

Add unique opaque types for the DELETE and INVALIDATE modifiers, so they are not any. This helps make the @typescript-eslint/no-unsafe-return rule happy.
Add a generic type parameter for the Modifiers type to get type inference for fields names and values.

I described the changes in the commits. Look at their descriptions for more information.

Both changes only affect TypeScript types. There are no runtime changes.

I understand the 2nd commit is riskier than the first commit. I can split it into 2 PRs if you want to take longer with the review.

Checklist:

If this PR contains changes to the library itself (not necessary for e.g. docs updates), please include a changeset (see CONTRIBUTING.md)
If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes) small feature, does not apply
Make sure all of the significant new logic is covered by tests

apollo-cla · 2023-05-19T11:31:30Z

@Gelio: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

netlify · 2023-05-19T11:31:30Z

👷 Deploy request for apollo-client-docs pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	`6973cd4`

changeset-bot · 2023-05-19T11:31:31Z

🦋 Changeset detected

Latest commit: fc50711

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@apollo/client	Minor

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

jerelmiller · 2023-05-19T22:29:22Z

Hey @Gelio 👋

Thanks so much for this PR! I'd like to talk about this a bit more with the team and pull this down to assess the backwards compatibility. We should be able to take a look at it next week. Thanks so much for helping make this library better for TypeScript users!

Gelio · 2023-05-20T11:35:14Z

That's great! I'm looking forward to your feedback

As far as backwards compatibility goes, both commits should be transparent to end users who haven't added any type annotations themselves in the past. The default types are still any. It is only those who added manual type annotations that will be impacted, unfortunately.

phryneas

Heyo, thanks a ton for the PR - it definitely makes things a lot better already. I have added a few comments. We might consider going a step further than this for easier discoverability.

phryneas · 2023-05-30T12:30:49Z

src/cache/core/types/common.ts

+  ? StoreVal | Reference
+  : StoreVal;
+
+export type Modifiers<T extends Record<string, unknown>> = Partial<{


This should have a default value to be backwards compatible.

Suggested change

export type Modifiers<T extends Record<string, unknown>> = Partial<{

export type Modifiers<T extends Record<string, unknown> = Record<string, unknown>> = Partial<{

Good idea 👍 I did that

phryneas · 2023-05-30T12:59:58Z

src/cache/core/types/common.ts

+declare const _deleteModifier: unique symbol;
+export type DeleteModifier = typeof _deleteModifier
+declare const _invalidateModifier: unique symbol;
+export type InvalidateModifier = typeof _invalidateModifier


Suggested change

declare const _deleteModifier: unique symbol;

export type DeleteModifier = typeof _deleteModifier

declare const _invalidateModifier: unique symbol;

export type InvalidateModifier = typeof _invalidateModifier

declare const _deleteModifier: unique symbol;

export interface DeleteModifier { [_deleteModifier]: true }

declare const _invalidateModifier: unique symbol;

export interface InvalidateModifier { [_invalidateModifier]: true}

Before this change:

After:

I noticed that earlier but didn't come up with a way of fixing it. Thanks for a ready-made snippet! I used that in the code

phryneas · 2023-05-30T13:07:20Z

src/cache/core/types/Cache.ts

@@ -59,7 +59,7 @@ export namespace Cache {

  export interface ModifyOptions {


Have you considered adding a generic to ModifyOptions and adding an overload using that cache.modify?

While I love the extra safety we are already getting here through satisfies Modifiers<Book>, it is not a very discoverable solution, so it might be an idea to explore that additional step.

I like the idea. I added a generic parameter to modify with any as the default value. It seems to work good enough and is backwards compatible, as far as I can tell.

My previous attempt without the `any` default type

So far, I added the generic like so:

export interface ModifyOptions<T> { id?: string; fields: T extends Record<string, unknown> ? Modifiers<T> | Modifier<T> : Modifier<T>; optimistic?: boolean; broadcast?: boolean; }

I added the extends Record<string, unknown> conditional type because Modifiers expect its generic parameter to extend Record<string, unknown>.

I believe that this conditional type makes type inference fail to infer the type of T based on the fields object:

declare function modify<T>(options: ModifyOptions<T>): void; modify({ fields: { bar: (value, options) => { return value; } } })

T is inferred as unknown, whereas it could be inferred to { bar: unknown }.

Thus, looks like a default type of any is needed to make this change backwards-compatible.

Gelio · 2023-05-30T16:32:53Z

Thanks for your review! I addressed the comments. I haven't rebased on main yet to make the latest additions easier to review. I am happy to rebase the branch whenever it's necessary

phryneas

Looks good, thank you!

Just one minor nitpick - maybe we can avoid that any there :)

phryneas · 2023-05-31T12:42:43Z

src/cache/inmemory/inMemoryCache.ts

@@ -204,7 +204,7 @@ export class InMemoryCache extends ApolloCache<NormalizedCacheObject> {
    }
  }

-  public modify(options: Cache.ModifyOptions): boolean {
+  public modify<Entity = any>(options: Cache.ModifyOptions<Entity>): boolean {


Maybe we could go for Record<string, any> as a default here - or am I missing any edge case where modify could be called on something that is not an object?

You are right, good point 👍 I made that change in a separate commit

Gelio · 2023-06-02T09:04:12Z

I rebased the branch on top of the latest main. It was a clean rebase, without modifying any existing commits. There is only one new commit since the last time @phryneas reviewed the PR

Problem: The `DELETE` and `INVALIDATE` modifiers type is `any`. Returning them from the `Modifier<T>` function triggers the `@typescript-eslint/no-unsafe-return` ESLint rule [0], since the modifier is then returning a value with type `any`. Solution: Set the types of these modifiers to two different `unique symbol`s. These serve as unique opaque types. The consuming code should not peek into them. These 2 types are permitted in the return type of the `Modifier<T>` function, since they are not `any` anymore, and thus, are not assignable to `T`. This change only affects TypeScript types. It has no runtime impact. [0]: https://typescript-eslint.io/rules/no-unsafe-return/

Problem: The `fields` property in `cache.modify` did not offer ways to infer the field names or values based on the field type. It accepted any field names and the field values were `any`. Solution: Add a generic type parameter for the object type to the `Modifiers` type. The code can now use the `satisfies Modifiers<...>` operator to inform TypeScript about the possible field names and values. Field values include `Reference`s for objects and arrays. The consuming code is then responsible for checking (or asserting) that the field value is or is not a reference.

Makes the change backwards-compatible.

Interfaces retain their names in error messages and hover dialogs. Type aliases did not.

A generic parameter is easier to discover and set than `fields: { ... } satisfies Modifiers<...>`

Use a more appropriate real-life default type for `cache.modify`.

phryneas · 2023-06-12T12:26:24Z

Just looking at this, I'm gonna add some more changes and rebase this on 3.8

One thing that I noticed:

extends Record<string, unknown> only matches type, not interface declarations - so I have to fall back to extends Record<string, any> again, sadly.

Gelio · 2023-06-12T12:28:37Z

Right, I always had trouble with these 2 variants of extends. Thanks for taking care of this PR 👍 Let me know if you need any assistance from me

phryneas · 2023-06-12T13:16:52Z

src/cache/inmemory/__tests__/cache.ts

@@ -2817,7 +2817,7 @@ describe("InMemoryCache#modify", () => {

    cache.modify({
      fields: {
-        comments(comments: Reference[], { readField }) {
+        comments(comments: readonly Reference[], { readField }) {


This change might introduce a bit of churn for our users (but for good reason) - instead of allowing Reference[] | ObjectType[] here, we pass these arrays in as ReadonlyArray.

We should talk about this at the meeting tomorrow.

phryneas · 2023-06-12T13:20:51Z

I've given this a rebase on 3.8 and a force-push.

@Gelio I have also introduced some changes - could you please give my review a review?

General changes:

added tests
extends Record<string, unknown> => extends Record<string, any> to match interface and type declarations
field changed from Modifier<Entity> to AllFieldsModifier<Entity>, which is a modifier function that essentially accepts all possible prop values of Entity, as that field function will be called for all the fields on Entity, not the Entity itself.
Array types are now readonly (this might introduce some churn & needs internal discussion)

Gelio

Thanks for taking care of this PR 👍 I especially like the types tests you added.

I added some comments. I believe only the one in src/cache/inmemory/types.ts is worth taking care of before the merge. Others are non-blocking and suggestions for improvements.

Let me know if you want me to address them. I thought I would just bring them up so you can have your say before we modify the code more.

.changeset/afraid-zebras-punch.md

src/cache/inmemory/__tests__/cache.ts

src/cache/core/types/common.ts

Gelio · 2023-06-12T15:15:36Z

src/cache/core/__tests__/cache.ts

@@ -2,7 +2,7 @@ import gql from 'graphql-tag';
 import { ApolloCache } from '../cache';
 import { Cache, DataProxy } from '../..';
 import { Reference } from '../../../utilities/graphql/storeUtils';
-
+import { expectTypeOf } from 'expect-type'


I am afraid tests are excluded from typechecking. I intentionally created a typechecking error and npm run build passed

Am I missing something?

If not, then perhaps we should start also typechecking tests to catch errors there. It would probably be safest to do in a separate PR. I'm just bringing it up here as I noticed a possible improvement (or a lack of understanding on my part)

They are from building, but they are checked when running the tests - also in CI.
See this CI run: https://app.circleci.com/pipelines/github/apollographql/apollo-client/20241/workflows/ece99794-faac-4ed0-b938-f8fe5e2b697c/jobs/102866

I see. Thanks for the info. I missed that typechecking happens during tests.

n cases like this one, I usually see 2 TS projects: one for the source code, one for the tests. The build typechecking (that happens in npm run build) can only compile the code project, while there could be a separate npm script that typechecks both projects. This way the typechecking would be easier to discover. Just some food for thought from an external contributor 🙂

@Gelio I'm late to the party, but figured I'd offer you (and others that stumble upon this comment) an explanation as to why you're seeing this.

We deliberately disabled strict type checking in development (#10268) because we wanted the ability to comment out code when debugging issues against failing tests while allowing our tests to run with broken types. Sometimes the easiest way to figure out why something is failing is to comment out specific parts of code to figure out what line of code is causing the issue. Debugging like this in some cases cascaded all over the place (I remember one of these to spot check a specific line of code and I ended up having to comment 50-60 lines of code just to get the test to run).

Instead, we allow TypeScript errors, but have it just show a warning at the very top of the tests. This allows us to see there are type issues, but our tests will still run. As @phryneas said, we have strict type checking when running in CI, so it will be caught in PRs and such.

Hope this helps!

@jerelmiller Thanks for a thorough explanation!

I understand the problem you were trying to solve. I would also be frustrated at the tests not attempting to run because there are some unused symbols in the code

A solution I see more commonly used in the wild is fully separating the typechecking from testing. I don't see much of the reason for jest (or ts-jest) to do both. As a fellow engineer, I would much rather have a dedicated npm script for typechecking, and a separate one for just running the tests (without checking the types). That would be less of a surprise than the current state of affairs 🙂

Maybe you would also get faster feedback this way, since jest would only strip the types and not have to compile the project first. This opens up possibilities of using a different jest transpiler, like @swc/jest, which could be faster at transpiling TS than tsc

I thought I would share this perspective of a first-time contributor.

src/cache/core/types/common.ts

src/cache/inmemory/types.ts

phryneas · 2023-06-12T15:37:41Z

Please go ahead and continue fixing things here - thank you for working with us on this :)

Use an already installed library instead of introducing a new utility function.

Promote the use of a generic parameter of `cache.modify` over using `satisfies` which requires importing the `Modifers` type.

Gelio · 2023-06-12T18:10:14Z

I'm happy to contribute :) I've just applied to suggestions and pushed 3 commits to the branch. The code looks good to me now

phryneas

Looks good to me now! Before we get this merged, I'll discuss the change to ReadonlyArray internally to get a team decision on that part.

Gelio · 2023-06-13T16:08:08Z

@phryneas Thanks for merging the PR 🎉

phryneas · 2023-06-14T09:45:59Z

@Gelio thank you for putting the work in and opening the PR in the first place! 😊

Gelio force-pushed the type-safe-cache-modifiers branch from 088691e to bbc2cea Compare May 19, 2023 11:39

jerelmiller added the ⛑ TypeScript label May 19, 2023

jerelmiller requested a review from phryneas May 23, 2023 15:25

phryneas reviewed May 30, 2023

View reviewed changes

Gelio requested a review from phryneas May 30, 2023 16:33

phryneas reviewed May 31, 2023

View reviewed changes

Gelio force-pushed the type-safe-cache-modifiers branch from 09a0528 to 3681939 Compare June 2, 2023 09:03

Gelio requested a review from phryneas June 2, 2023 09:04

bignimbus assigned phryneas Jun 6, 2023

Gelio added 6 commits June 12, 2023 13:54

refactor(cache): add default value for Modifiers generic parameter

4568407

Makes the change backwards-compatible.

refactor(cache): use interfaces for invalidate and delete modifier types

9f7d536

Interfaces retain their names in error messages and hover dialogs. Type aliases did not.

refactor(cache): add generic parameter for ModifyOptions

1cfb03b

A generic parameter is easier to discover and set than `fields: { ... } satisfies Modifiers<...>`

refactor(cache): use Record<string, any> as the default Entity type

014f17d

Use a more appropriate real-life default type for `cache.modify`.

code review adjustments

6973cd4

phryneas force-pushed the type-safe-cache-modifiers branch from 3681939 to 6973cd4 Compare June 12, 2023 13:14

phryneas requested a review from StephenBarlow as a code owner June 12, 2023 13:14

phryneas changed the base branch from main to release-3.8 June 12, 2023 13:14

phryneas removed the request for review from StephenBarlow June 12, 2023 13:14

phryneas reviewed Jun 12, 2023

View reviewed changes

Gelio commented Jun 12, 2023

View reviewed changes

Gelio added 3 commits June 12, 2023 19:58

refactor(cache-test): replace assertType with expect-type library

4c12626

Use an already installed library instead of introducing a new utility function.

chore: use generic parameter instead of satisfies in changeset

18e8bf9

Promote the use of a generic parameter of `cache.modify` over using `satisfies` which requires importing the `Modifers` type.

fix(cache): use AllFieldsModifier type in cache.modify parameter

fc50711

Gelio requested a review from phryneas June 12, 2023 18:10

phryneas approved these changes Jun 13, 2023

View reviewed changes

phryneas merged commit e187866 into apollographql:release-3.8 Jun 13, 2023

github-actions bot mentioned this pull request Jun 13, 2023

Version Packages (beta) #10963

Merged

Gelio deleted the type-safe-cache-modifiers branch June 13, 2023 16:08

github-actions bot locked as resolved and limited conversation to collaborators Jul 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve TypeScript type-safety of `cache.modify` #10895

Improve TypeScript type-safety of `cache.modify` #10895

Gelio commented May 19, 2023

apollo-cla commented May 19, 2023

netlify bot commented May 19, 2023 •

edited

Loading

changeset-bot bot commented May 19, 2023 •

edited

Loading

jerelmiller commented May 19, 2023

Gelio commented May 20, 2023

phryneas left a comment

phryneas May 30, 2023

Gelio May 30, 2023

phryneas May 30, 2023

Gelio May 30, 2023 •

edited

Loading

phryneas May 30, 2023

Gelio May 30, 2023

Gelio commented May 30, 2023

phryneas left a comment

phryneas May 31, 2023 •

edited

Loading

Gelio Jun 2, 2023

Gelio commented Jun 2, 2023

phryneas commented Jun 12, 2023

Gelio commented Jun 12, 2023

phryneas Jun 12, 2023

phryneas commented Jun 12, 2023 •

edited

Loading

Gelio left a comment

Gelio Jun 12, 2023

phryneas Jun 12, 2023

Gelio Jun 12, 2023

jerelmiller Jun 20, 2023

Gelio Jun 21, 2023 •

edited

Loading

phryneas commented Jun 12, 2023

Gelio commented Jun 12, 2023

phryneas left a comment

Gelio commented Jun 13, 2023

phryneas commented Jun 14, 2023

	export type Modifiers<T extends Record<string, unknown>> = Partial<{
	export type Modifiers<T extends Record<string, unknown> = Record<string, unknown>> = Partial<{

		@@ -59,7 +59,7 @@ export namespace Cache {

		export interface ModifyOptions {

Improve TypeScript type-safety of cache.modify #10895

Improve TypeScript type-safety of cache.modify #10895

Conversation

Gelio commented May 19, 2023

Checklist:

apollo-cla commented May 19, 2023

netlify bot commented May 19, 2023 • edited Loading

👷 Deploy request for apollo-client-docs pending review.

changeset-bot bot commented May 19, 2023 • edited Loading

🦋 Changeset detected

jerelmiller commented May 19, 2023

Gelio commented May 20, 2023

phryneas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gelio May 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gelio commented May 30, 2023

phryneas left a comment

Choose a reason for hiding this comment

phryneas May 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gelio commented Jun 2, 2023

phryneas commented Jun 12, 2023

Gelio commented Jun 12, 2023

Choose a reason for hiding this comment

phryneas commented Jun 12, 2023 • edited Loading

Gelio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gelio Jun 21, 2023 • edited Loading

Choose a reason for hiding this comment

phryneas commented Jun 12, 2023

Gelio commented Jun 12, 2023

phryneas left a comment

Choose a reason for hiding this comment

Gelio commented Jun 13, 2023

phryneas commented Jun 14, 2023

Improve TypeScript type-safety of `cache.modify` #10895

Improve TypeScript type-safety of `cache.modify` #10895

netlify bot commented May 19, 2023 •

edited

Loading

changeset-bot bot commented May 19, 2023 •

edited

Loading

Gelio May 30, 2023 •

edited

Loading

phryneas May 31, 2023 •

edited

Loading

phryneas commented Jun 12, 2023 •

edited

Loading

Gelio Jun 21, 2023 •

edited

Loading