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

Update Context Types #12104

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

samchungy
Copy link

@samchungy samchungy commented Oct 25, 2024

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)
  • Make sure all of the significant new logic is covered by tests

The tl;dr

  • The context type on all operations must now extend OperationContext (Record<string, any>) and defaults to Partial<DefaultContext>
  • The ApolloClient constructor context field now requires a non partial DefaultContext.

In Depth

I was trying to work with @phryneas comment here: apollographql/apollo-client-nextjs#103 (comment)

by setting a defaultContext type:

eg.

declare module '@apollo/client' {
  export interface DefaultContext {
    appContext: {
      foo: string
    }
    triggerBehaviour?: boolean
  }
}

However, this had cascading effects to the other operations such as useQuery where suddenly the operation inputs were expecting appContext to be in the context which is incorrect as we should only require a partial of the default context to allow for overwriting

So I decided to go and fix the query type but realised the mutation type allows the Context to be set:

context?: TContext;

So in the name of consistency I went to try and add generics for the Query types and many changes later here we are.

Copy link

netlify bot commented Oct 25, 2024

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

Visit the deploys page to approve it

Name Link
🔨 Latest commit 4fe7305

@svc-apollo-docs
Copy link

svc-apollo-docs commented Oct 25, 2024

✅ Docs Preview Ready

No new or changed pages found.

Copy link

changeset-bot bot commented Oct 25, 2024

🦋 Changeset detected

Latest commit: 4fe7305

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 Patch

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

@@ -474,7 +475,7 @@ export class ApolloClient<TCacheShape> implements DataProxy {
public mutate<
TData = any,
TVariables extends OperationVariables = OperationVariables,
TContext extends Record<string, any> = DefaultContext,
TContext extends OperationContext = Partial<DefaultContext>,
Copy link
Author

Choose a reason for hiding this comment

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

I created a new type called OperationContext to represent Record<string, any>. I wanted to call it Context but

export type { DefaultContext as Context } from "../../core/index.js";
that name is already taken

Comment on lines +14 to +16
export function setContext<
TContext extends OperationContext = Partial<DefaultContext>,
>(setter: ContextSetter<TContext>): ApolloLink {
Copy link
Author

Choose a reason for hiding this comment

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

Update the types here to reflect that users only need to pass in a partial of the context.

const setContext: Operation["setContext"] = (next) => {
if (typeof next === "function") {
context = { ...context, ...next(context) };
} else {
context = { ...context, ...next };
}
};
const getContext: Operation["getContext"] = () => ({ ...context });

But I also thought that users might want to strictly type what they want to return here so I've given the flexibility to pass in TContext.

query,
fetchPolicy,
errorPolicy = "none",
variables,
context = {},
context = {} as TContext,
Copy link
Author

Choose a reason for hiding this comment

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

I'm not particularly sure what the defaulting is for here but I decided to leave it in to avoid any breaking change and am choosing to cast here instead.

@samchungy samchungy changed the title Optimise context type Update Context Types Oct 25, 2024
@samchungy samchungy marked this pull request as ready for review October 25, 2024 07:40
@samchungy
Copy link
Author

Not sure why the tests are failing as there are no code changes in this PR

@samchungy
Copy link
Author

samchungy commented Oct 28, 2024

Should this be required? If we're typing DefaultContext, I would expect this to be filled on client creation?

defaultContext?: Partial<DefaultContext>;

If they need it to be optional they could declare it themselves while extending it

@phryneas
Copy link
Member

Hi Sam,
I just wanted to leave quick feedback: we're all just back from conferences and will likely need a few days to catch up on everything. The PR is very welcome, but I'll need to free up some headspace to look at it thoroughly :)

@samchungy
Copy link
Author

samchungy commented Oct 30, 2024

Hi Sam, I just wanted to leave quick feedback: we're all just back from conferences and will likely need a few days to catch up on everything. The PR is very welcome, but I'll need to free up some headspace to look at it thoroughly :)

No problems, we're all busy people! Fully understand, thanks for the reply

@@ -116,7 +117,7 @@ export interface ApolloClientOptions<TCacheShape> {
* See this [example object](https://www.apollographql.com/docs/react/api/core/ApolloClient#example-defaultoptions-object).
*/
defaultOptions?: DefaultOptions;
defaultContext?: Partial<DefaultContext>;
defaultContext?: DefaultContext;
Copy link
Author

Choose a reason for hiding this comment

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

I think this makes sense, if you are expecting something to always be in here, you would declare it as Required on the default context?

Unless we're intentionally making this Partial so you could type things like headers: Record<string, string> as Apollo seems to always inject these

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants