-
Notifications
You must be signed in to change notification settings - Fork 35
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
Use a symbol property instead of instanceOf to check for correct ApolloClient/InMemoryCache #302
Conversation
#211 Bundle Size — 1.07MiB (+6.54%).Warning Bundle contains 1 duplicate package – View duplicate packages Bundle metrics
|
Current #211 |
Baseline #189 |
|
---|---|---|
Initial JS | 939.02KiB (+5.33% ) |
891.55KiB |
Initial CSS | 70B (+100% ) |
0B |
Cache Invalidation | 85.99% |
0.04% |
Chunks | 32 (+33.33% ) |
24 |
Assets | 56 (+24.44% ) |
45 |
Modules | 591 (+15.43% ) |
512 |
Duplicate Modules | 110 (+266.67% ) |
30 |
Duplicate Code | 7.61% (+489.92% ) |
1.29% |
Packages | 26 (-10.34% ) |
29 |
Duplicate Packages | 1 |
1 |
Bundle size by type 3 changes
3 regressions
Current #211 |
Baseline #189 |
|
---|---|---|
JS | 1.06MiB (+6.31% ) |
1MiB |
Other | 8.67KiB (+44.68% ) |
5.99KiB |
CSS | 70B (+100% ) |
0B |
Bundle analysis report Branch pr/drop-instanceof Project dashboard
if (!(clientRef.current instanceof ApolloClient)) { | ||
throw new Error( | ||
`When using \`ApolloClient\` in streaming SSR, you must use the \`${WrappedApolloProvider.info.client}\` export provided by \`"${WrappedApolloProvider.info.pkg}"\`.` | ||
assertInstance( |
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.
No need to do this check on every render, doing it in the one render that initializes clientRef
is enough.
const wrappers = Symbol.for("apollo.hook.wrappers"); | ||
class ApolloClientBase<TCacheShape> extends OrigApolloClient<TCacheShape> { | ||
class ApolloClientBase extends OrigApolloClient<NormalizedCacheObject> { |
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.
Since we now only accept InMemoryCache
in the types (this was already the case before in runtime), that type argument will always be NormalizedCacheObject
- it's inferred from the cache.
That also means we can drop the generic from all the internal implementations.
/** | ||
* Information about the current package and it's export names, for use in error messages. | ||
* | ||
* @internal | ||
*/ | ||
static readonly info = bundle; | ||
|
||
constructor(options: ApolloClientOptions<TCacheShape>) { | ||
[sourceSymbol]: string; |
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 real core of this PR. Sorry for all the other noise!
Partial<ApolloClientBrowserImpl<TCacheShape>>, | ||
Partial<ApolloClientSSRImpl<TCacheShape>> | ||
export class ApolloClient< | ||
// this generic is obsolete as we require a `InMemoryStore`, which fixes this generic to `NormalizedCacheObject` anyways |
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.
Keeping a generic here for backwards compat, but we won't do anything with it.
import { outsideOf } from "@internal/test-utils/runInConditions.js"; | ||
import { browserEnv } from "@internal/test-utils/react.js"; | ||
import { silenceConsoleErrors } from "@internal/test-utils/console.js"; |
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.
Since we now have tests in both bundles, I've moved common test util code into a new package, @internal/test-utils
.
const jsdom = await import("global-jsdom"); | ||
using _cleanupJSDOM = { [Symbol.dispose]: jsdom.default() }; |
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.
JSDom setup and other preparations moved into browserEnv
.
* | ||
* @public | ||
*/ | ||
export class InMemoryCache extends UpstreamInMemoryCache { |
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.
We add our own implementation here to ensure the error messages mention this package, not the parent package.
export function silenceConsoleErrors(): { | ||
[Symbol.dispose](): void; | ||
}; |
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 committing the generated .d.ts files here - setting up something with project references doesn't work nicely with tsx
, and honestly it's not worth the hassle.
@@ -0,0 +1,9 @@ | |||
export function silenceConsoleErrors() { |
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.
As a tradeoff, these are now .js files with JsDoc, not .ts files.
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.
LGTM!
Co-authored-by: Jerel Miller <jerelmiller@gmail.com>
No description provided.