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

Make keyFields a readonly array #9339

Merged
merged 6 commits into from
Feb 10, 2022
Merged

Make keyFields a readonly array #9339

merged 6 commits into from
Feb 10, 2022

Conversation

julienfouilhe
Copy link
Contributor

Just a small typing change.

The current behaviour prevents doing things like this:

const apolloMainTypePolicies = {
  User: {
    keyFields: ['id'],
  },
} as const;

export type ApolloMainTypePolicies = typeof apolloMainTypePolicies;

export const getCacheForApollo = () => {
  return new InMemoryCache({
    possibleTypes: introspectionQueryResultData.possibleTypes,
    typePolicies: apolloMainTypePolicies,
    // Type 'readonly ["id"]' is not assignable to type 'false | KeySpecifier | KeyFieldsFunction | undefined'.
    //    The type 'readonly ["id"]' is 'readonly' and cannot be assigned to the mutable type 'KeySpecifier'.
  });
};

@apollo-cla
Copy link

@julienfouilhe: 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/

Copy link
Member

@benjamn benjamn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable, especially since mutating these arrays is unlikely to produce desirable results (whatever the goal might be). Thanks @julienfouilhe!

@benjamn benjamn added this to the v3.5.x patch releases milestone Jan 21, 2022
@julienfouilhe
Copy link
Contributor Author

@benjamn Typescript seems to be having trouble discriminating KeySpecifier in some cases now though, I'll come up with something to fix this!

@benjamn benjamn self-requested a review January 21, 2022 19:37
@julienfouilhe
Copy link
Contributor Author

@benjamn I added a isReadonlyArray helper, it was the easiest/most simple solution I could find. Tell me if you think of something smarter!

Comment on lines 122 to 123

export const isReadonlyArray = (a: any): a is ReadonlyArray<any> => Array.isArray(a)
Copy link
Contributor

@sztadii sztadii Jan 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching the issue, but the name of the function is misleading.
It is not really filtering the read-only types but fixing the issue with the isArray method.

Screenshot 2022-01-26 at 19 41 08

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you rename the helper to isArray?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

export const isArray = (a: any): a is ReadonlyArray<any> | Array<any> => Array.isArray(a)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sztadii you're right, done!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@julienfouilhe I am only worried that someday TS will change the behavior of read-only arrays.
Sometimes maybe we will prefer to use a mutable array.
Can we change isArray to something like that

export const isArray = (a: any): a is any[] | readonly any[] => Array.isArray(a)

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sztadii done! 👍

Copy link
Member

@benjamn benjamn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks again @julienfouilhe!

@benjamn benjamn merged commit 4e147b5 into apollographql:main Feb 10, 2022
@julienfouilhe julienfouilhe deleted the readonly-keyFields branch February 13, 2022 19:23
@benjamn
Copy link
Member

benjamn commented Feb 15, 2022

These changes should now be available from npm in @apollo/[email protected] (just published).

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants