-
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
Run Knip on 4.0 branch #12291
Run Knip on 4.0 branch #12291
Conversation
🦋 Changeset detectedLatest commit: be729ad 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 |
|
commit: |
commit: |
size-limit report 📦
|
/** | ||
* @deprecated This function has no "resetting" effect since Apollo Client 3.4.12, | ||
* and will be removed in the next major version of Apollo Client. | ||
* If you want to get the Apollo Context, use `getApolloContext` instead. | ||
*/ | ||
export const resetApolloContext = getApolloContext; |
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 was already deprecated and knip complained (rightfully so) that it's a re-export of the same thing with two names - seemed like a good time to delete it.
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
package.json
Outdated
@@ -100,7 +101,6 @@ | |||
"graphql-tag": "^2.12.6", | |||
"optimism": "^0.18.0", | |||
"rehackt": "^0.1.0", | |||
"response-iterator": "^0.2.6", |
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 an actual dependency that we apparently never referenced, since the relevant parts seem to be inlined in our code.
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.
Good catch. Should we do this on the main
branch as well?
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.
Good idea, that package pulls in quite a lot of other dependencies.
@@ -118,7 +118,11 @@ | |||
"@graphql-tools/merge": "9.0.4", | |||
"@graphql-tools/schema": "10.0.4", | |||
"@graphql-tools/utils": "10.5.0", | |||
"@jest/expect-utils": "29.7.0", |
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.
Knip pointed out a bunch of devDependencies that we actually never directly installed, but referenced in our code, so those were added.
Also, some that we never referenced, so they were removed.
@@ -1,6 +1,5 @@ | |||
// These hooks are used internally and are not exported publicly by the library | |||
export { useDeepMemo } from "./useDeepMemo.js"; | |||
export { useIsomorphicLayoutEffect } from "./useIsomorphicLayoutEffect.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.
We reference this via a direct import from internal/useIsomorphicLayoutEffect.js
everywhere, so this re-export was apparently never used.
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.
Should we update those direct imports instead? I'm ok with either, but seems odd to only have one of these hooks referenced directly while the others are imported from this barrel file.
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.
I think we were doing this with a reason - a circular import issue or something.
src/cache/inmemory/helpers.ts
Outdated
@@ -31,7 +31,7 @@ import { | |||
|
|||
export const { hasOwnProperty: hasOwn } = Object.prototype; | |||
|
|||
export function isNullish(value: any): value is null | undefined { | |||
function isNullish(value: any): value is null | undefined { |
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.
Should we just replace usages of this function with value == null
since that does the same thing? I believe that is the one "blessed" usage of ==
.
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.
Sounds great.
@@ -133,7 +133,7 @@ export function parseHeaders(headerText: string): Record<string, string> { | |||
return headersInit; | |||
} | |||
|
|||
export function parseJsonBody<T>(response: Response, bodyText: string): T { | |||
function parseJsonBody<T>(response: Response, bodyText: string): T { |
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.
I know parseAndCheckHttpResponse
is surprisingly popular among 3rd party links. We should probably check a few to see if these are ever used. We may want to keep these exports (or at least call them out in the changelog if we don't)
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.
It will go away the second we add an exports
field as it will become impossible to import from non-official entry points.
If we want to keep it, we need to start exporting it from an official entry point - I'd be totally open to that.
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.
On second look, this looks like an implementation detail of parseAndCheckHttpResponse
- even we don't use it outside of this file, so I'd just remove the export until someone complains.
I am adding , (We already do) as it's at least used in https://github.com/jaydenseric/apollo-upload-client/blob/5e6d5c17861fbd9e2d84c8517fc29d5321be4cfc/createUploadLink.mjs#L5 - we need to remember to PR them an import change when we release 4.0.parseAndCheckHttpResponse
as an export from link/http
though
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.
Definitely one of those modules we'll need to call out well in the changelog. I think there are a few 3rd party link implementations that use this function.
@@ -42,7 +42,7 @@ export interface DefaultOptions { | |||
mutate?: Partial<MutationOptions<any, any, any>>; | |||
} | |||
|
|||
export interface DevtoolsOptions { | |||
interface DevtoolsOptions { |
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.
Do you think we should export this from @apollo/client
?
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.
I think we are probably ok to leave it not exported for now. Generally I'd expect the config to be passed to the constructor 99% of the time so I can't imagine too many people are needing this.
…rapping library might use
I've added an integration test that shows some level of "writing a wrapping library". It's not erroring now, but I imagine it will start erroring once we add an exports field, so this will be good to have. |
Generally, I'd suggest we get this merged as-is right now and we'll see if there are any problems down the road - we can always add more |
@@ -207,7 +207,7 @@ export interface SubscriptionOptions< | |||
extensions?: Record<string, any>; | |||
} | |||
|
|||
export interface MutationBaseOptions< | |||
interface MutationBaseOptions< |
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 will likely disappear once we flatten out our types and localize some of these with their respective hooks, etc. Totally ok with this change.
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.
The things you find hiding in the codebase sometimes 🤯
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.
Good stuff! Thanks for getting this integrated.
I'll get this merged - if we get user feedback that they need type X, we can always add it back, but this seems like a good clean start. |
This runs
knip --fix
on the 4.0 branch.Some of these solutions might need more discussions:
Knip removed all type exports that were not accessible through our official exports. Right now, these can technically be imported from by importing them from our
dist
folder, but they would become inaccessible if we added anexports
field.We need to make a per-case decision on each of those if we either export them from an official entrypoint or stop exporting them.
We should probably err more on the side of "export them via official entry point", as otherwise libraries and library-like code reliant on these types might break upon build with the
declarations: true
tsconfig
flag.