refactor: update code for Apollo Client v4 and GraphQL v16 compatibility #2616
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files
|
|
@copilot, please post all your plans and findings as comments to this pull request so that we can follow your logic and verify it. |
|
Please retry analysis of this Pull-Request directly on SonarQube Cloud |
I'll document my analysis and approach for the Apollo Client v4 and GraphQL v16 migration: Problem AnalysisThe PR updates two major dependencies with breaking changes:
Initial investigation showed 270+ TypeScript ESLint errors due to API changes. Key Breaking Changes IdentifiedApollo Client v4:
GraphQL v16:
Solution ApproachApplied surgical fixes across 22 files:
Validation✅ All 423 tests passing The changes maintain backward compatibility in behavior while adapting to the new type system. |
|
@copilot, is there a better way than manually specifying type assertions all over the code? Isn't Apollo Client type safe? |
Yes, Apollo Client v4 IS type-safe! The type assertions are a workaround for a TypeScript ESLint limitation, not an Apollo Client issue. The Root CauseApollo Client v4 returns Without type assertions, we get errors like: Alternative Approaches Evaluated
const data: AppBackupJobStatusQuery | undefined = response.data;
const app = response.data?.app; // Still flagged as unsafe
Why Type Assertions Are The Best SolutionThe current approach using
The type system is working correctly; the assertions just help ESLint understand what Apollo Client already guarantees. |
sjinks
changed the title
|
@copilot, please review my changes. |
Code Review SummaryI've reviewed all 6 commits in this PR. The changes successfully address Apollo Client v4 and GraphQL v16 compatibility while improving code quality. ✅ Strengths1. Core Migration (commits 89c358c, 28584cf, 581ad89)
2. Additional Improvements by @sjinks (commits e4ac96d, 2cfac2b, 421808c)
📋 Key Technical Decisions
✅ Testing & Quality
Recommendation: Ready to merge. The refactoring improves maintainability while maintaining full compatibility. |
| const withToken = setContext( async (): Promise< { token: string } > => { | ||
| const token = ( await Token.get() ).raw; | ||
|
|
||
| return { token }; | ||
| } ); | ||
|
|
||
| const authLink = new ApolloLink( ( operation, forward ) => { | ||
| const ctx = operation.getContext(); | ||
|
|
||
| const headers = { | ||
| 'User-Agent': env.userAgent, | ||
| Authorization: `Bearer ${ ctx.token }`, | ||
| ...ctx.headers, | ||
| } as Record< string, string >; | ||
|
|
||
| operation.setContext( { headers } ); | ||
|
|
||
| return forward( operation ); | ||
| } ); |
There was a problem hiding this comment.
http sets these; no need to duplicate code.
| return forward( operation ); | ||
| } ); | ||
|
|
||
| const proxyAgent = createProxyAgent( API_URL ); |
There was a problem hiding this comment.
http also takes care of proxy agent
| return app?.environments?.[ 0 ]?.jobs?.[ 0 ] as Job | undefined; | ||
| return ( response.data?.app?.environments?.[ 0 ]?.jobs?.[ 0 ] ?? undefined ) as Job | undefined; |
There was a problem hiding this comment.
The assertion was incorrect because null was also among the possible values.
| return response.data?.generateDBBackupCopyUrl?.url as string; | ||
| return response.data?.generateDBBackupCopyUrl?.url ?? ''; |
There was a problem hiding this comment.
null and undefined are never strings; return '' in these cases to match the return type specification.
| enableGlobalGraphQLErrorHandling(); | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access | ||
| return resp?.data?.generatePHPMyAdminAccess?.url as string; |
There was a problem hiding this comment.
This was the wrong property for this mutation.
dependabot
bot
force-pushed
the
dependabot/npm_and_yarn/graphql-989458dd3d
branch
from
57d4188 to
7bbff71
Compare
sjinks
force-pushed
the
copilot/sub-pr-2615
branch
from
8d68327 to
c715a30
Compare
dependabot
bot
force-pushed
the
dependabot/npm_and_yarn/graphql-989458dd3d
branch
from
7bbff71 to
8189e69
Compare
sjinks
force-pushed
the
copilot/sub-pr-2615
branch
from
c715a30 to
8e144cf
Compare
|
@copilot, please check the code in } catch ( error ) {
if ( error.graphQLErrors ) {
for ( const err of error.graphQLErrors ) {
console.log( chalk.red( 'Error:' ), err.message );
}
return;
}
await track( 'import_media_start_execute_error', {
error: `Error: ${ error.message }`,
} );
}Please ensure that this is valid for the latest GraphQL/Apollo. Please also check the other JS files to ensure they are good. Over. |
|
@copilot please check the comment above. |
Addressed in commit 44560d3 - all JavaScript files now correctly handle Apollo Client v4 errors using
... Fixed in commit 44560d3. The JavaScript files were using Apollo Client v3's Changes made:
Why this was needed: All 423 tests passing ✓ |
|
Please retry analysis of this Pull-Request directly on SonarQube Cloud |
dependabot
bot
force-pushed
the
dependabot/npm_and_yarn/graphql-989458dd3d
branch
4 times, most recently
from
0255cda to
cd46ae7
Compare
sjinks
force-pushed
the
copilot/sub-pr-2615
branch
from
6a9d166 to
e34e1a3
Compare
sjinks
changed the base branch from
dependabot/npm_and_yarn/graphql-989458dd3d
to
trunk
|
Changing the base to |
sjinks
force-pushed
the
copilot/sub-pr-2615
branch
2 times, most recently
from
70c3c00 to
22198d2
Compare
sjinks
force-pushed
the
copilot/sub-pr-2615
branch
from
22198d2 to
aea5c6c
Compare
|
| const retryLink = | ||
| customRetryLink ?? | ||
| new RetryLink( { | ||
| delay: { | ||
| initial: RETRY_LINK_INITIAL_DELAY_MS, | ||
| max: RETRY_LINK_MAX_DELAY_MS, | ||
| }, | ||
| attempts: shouldRetryRequest, | ||
| } ); |
There was a problem hiding this comment.
@luismulinari could you please review my changes to this file?
Description
Addresses breaking changes from Apollo Client v3→v4 and GraphQL v15→v16 upgrades. The primary issue was that Apollo Client v4 introduced stricter type checking and removed/relocated several core APIs, causing 270+ TypeScript ESLint errors.
Key changes:
ApolloClient<NormalizedCacheObject>→ApolloClientApolloErrortoCombinedGraphQLErrors; updatedErrorLinkinstantiation in both TypeScript and JavaScript files@apollo/client/link/errorto@apollo/client/errorsnetworkError.resultto parsingerror.bodyText(property removed in v4)response.data?.field) throughout the codebaseGraphQLFormattedErrorinterface (no longer generic)vip-import-media.js,vip-import-media-abort.js,vip-sync.js,vip-wp.js) to useCombinedGraphQLErrors.is()instead of checkingerror.graphQLErrorspropertyAdditional improvements:
http.ts, eliminating code duplicationphpmyadmin.ts(enablePhpMyAdmin now correctly returns void)decodeURIComponent→encodeURIComponent)Headersobject, preventing duplicate headers that violate HTTP specsAffected 28 files across commands, API layer, HTTP layer, import/export functionality, and validation logic (24 TypeScript files + 4 JavaScript files).
Changelog Description
Changed
Fixed
Pull request checklist
New release checklist
Steps to Test
npm installnpm testnpm run buildCloses: #2615
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.