-
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
Add a defaultContext property on ApolloClient #11275
Conversation
🦋 Changeset detectedLatest commit: c168bd2 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 |
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.
Couple questions for you, but this looks good otherwise. I'd be happy to approve once you have a chance to respond!
expect(context).toEqual( | ||
expect.objectContaining({ | ||
foo: { bar: "baz" }, | ||
a: { b: undefined, x: "y" }, |
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.
Is the b: undefined
here actually correct? Since its a shallow merge, I'd expect a
to just be { x: 'y' }
with the b
key completely omitted. Would it be useful to make sure this test checks that? I think it passes right now because technically the value of b
is undefined
in both cases, but it feels misleading to show it here since this makes it look like it does some kind of deep merge.
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 just my way of convincing jest
to make sure that b
isn't there or has no value.
If I tested for a: { x: "y" },
jest would also accept a a: { b: "still here", x: "y" },
as we are using objectContaining
(the real context has a bunch of other values I don't care about and that I don't want to specify in this test)
); | ||
}); | ||
|
||
it("`defaultContext` will be shallowly merged with context from `defaultOptions.query.context", async () => { |
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.
Actually this test brings up a good question:
What is the value of a defaultContext
on its own rather than using defaultOptions.<query|watchQuery>.context
? I'd like to better understand why someone couldn't just use defaultOptions
.
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 for completeness (we talked about this in person) - the defaultOptions
would overwrite context instead of shallow-merging it, and also require people to save their token to three different places instead of having it in one central position (so a big DX improvement).
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.
Chatted in person and the big change that this enables via a top-level defaultContext
is that it is shallow merged with incoming context, where the current query
/watchQuery
/mutate
default context is completely overwritten by incoming context. This was the crucial piece I missed in my initial review. Thanks for introducing this change!
size-limit report 📦
|
Getting this in and putting another alpha out :) |
This would add a convenient way to address an issue that I've seen come up a lot recently:
People don't have an "obvious" way to change their authentication token outside the link chain, apart from recreating their
ApolloClient
instance, losing all cache contents in the process.Our docs on authentication show using
localStorage
, which is not easily possible in Next.js, and might arguably be a bad practice in the first place.What's left are global variables (again, not in Next.js) or scoping tricks, e.g. using a
ref
- but this is not really obvious.Some recent related issues:
apollographql/apollo-client-nextjs#21
apollographql/apollo-client-nextjs#103
apollographql/apollo-client-nextjs#44
Also, half a dozen of Discord discussions.
To finish up the story, this would require an additional code change in the Next helper package to expose the
client
instance to parents of theNextApolloProvider
, possibly using a callback orref
.Checklist: