-
Notifications
You must be signed in to change notification settings - Fork 11
chore(types): add type check #374
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR performs a major dependency downgrade, changing Vitest from version 3.2.4 to 3.0.0, alongside TypeScript type safety improvements and code quality enhancements across multiple backend implementations (APISIX, API7) and the SDK. The changes include adding explicit type assertions, fixing type compatibility issues, and improving null safety handling throughout the codebase.
- Downgraded vitest from 3.2.4 to 3.0.0 with corresponding dependency adjustments
- Added type assertions and explicit casting to resolve TypeScript strict mode violations
- Refactored schema definitions to better support plugin configuration types
Reviewed Changes
Copilot reviewed 32 out of 34 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Downgraded vitest and related dependencies from 3.2.4 to 3.0.0, added package dependencies to apps/cli and libs/sdk |
| libs/sdk/src/core/schema.ts | Changed pluginsSchema from record type to single plugin schema, added UpstreamHealthCheck type export, extended ConfigurationSchema with new optional fields |
| libs/sdk/src/backend/index.ts | Made httpAgent and httpsAgent optional in BackendOptions |
| libs/backend-apisix/src/transformer.ts | Added extensive type assertions and null-safety checks throughout transformation methods |
| libs/backend-apisix/src/typing.ts | Changed GlobalRule and PluginMetadata from record types to direct types |
| libs/backend-api7/src/transformer.ts | Improved type safety with explicit return types and type assertions |
| libs/backend-api7/src/index.ts | Added non-null assertions and improved type guards in defaultValue method |
| apps/cli/src/utils/listr.ts | Replaced logical AND shortcuts with explicit if statements for better readability |
| apps/cli/package.json | Moved dependencies from devDependencies to dependencies |
| .github/workflows/lint.yaml | Added new GitHub Actions workflow for lint checking |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: 'lts/*' | ||
| - uses: pnpm/action-setup@v4 | ||
| - name: Install dependencies | ||
| run: pnpm install | ||
|
|
||
| - name: Run TypeScript Type Check | ||
| run: npx nx run-many --target=typecheck --all --skip-nx-cache |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
This autofix suggestion was applied.
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 days ago
To fix the problem, explicitly set the minimum required permissions by adding a permissions: block at the workflow or job level. Since no step appears to require token-based write access, the least privilege is contents: read. Place permissions: at the top level, just after the workflow name: and before on:, so it applies to all jobs unless overridden. No other code changes, imports, or definitions are needed.
-
Copy modified lines R2-R3
| @@ -1,4 +1,6 @@ | ||
| name: Typecheck | ||
| permissions: | ||
| contents: read | ||
| on: | ||
| push: | ||
| branches: |
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.
@LiteSun Please flow this guide.
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.
fixed
tsconfig.json
Outdated
| "compilerOptions": { | ||
| "lib": ["esnext"], | ||
| }, |
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.
Please revert this change: https://nx.dev/docs/concepts/typescript-project-linking#set-up-typescript-project-references
package.json
Outdated
| "url-loader": "^4.1.1", | ||
| "vite": "7.1.9", | ||
| "vitest": "^3.0.0", | ||
| "vitest": "3.0.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.
Always use the latest version of 3.x. Keep ^3.0.0.
| routes: z.array(routeSchema).optional(), | ||
| stream_routes: z.array(streamRouteSchema).optional(), | ||
| consumer_credentials: z.array(consumerCredentialSchema).optional(), | ||
| upstreams: z.array(upstreamSchema({ id: idSchema.optional() })).optional(), |
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.
These elements do not exist in the ADC schema and users are not permitted to configure these fields here. Do not modify the core schema.
If your type system requires these fields, create another type alias.
const AnotherConfiguration = z.infer<typeof ConfigurationSchema> & {xx: xx, yy: yy};
libs/sdk/src/backend/index.ts
Outdated
| cacheKey?: string; | ||
| httpAgent?: httpAgent; | ||
| httpsAgent?: httpsAgent; |
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.
They are mandatory. Every backend must provide them, and any E2E tests containing errors need to be fixed individually.
| "e2e/**/*.ts" | ||
| ] | ||
| } | ||
| } |
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.
PLEASE KEEP EOL
|
|
||
| private listGlobalRules() { | ||
| return this._list<typing.ListResponse<typing.GlobalRule>>( | ||
| return this._list<typing.ListResponse<Record<string, typing.GlobalRule>>>( |
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.
🤔
libs/backend-apisix/src/fetcher.ts
Outdated
| Object.fromEntries<ADCSDK.Plugin>( | ||
| list.map((item) => [ | ||
| item.key.split('/').pop(), | ||
| item.key.split('/').pop() ?? item.key, |
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'm concerned this might be incorrect, so it's best not to modify it.
The Admin API has some default rules, such as keys containing / being returned here.
| } | ||
| : undefined, | ||
| ssl_protocols: ssl.ssl_protocols, | ||
| ssl_protocols: ssl.ssl_protocols as ('TLSv1.1' | 'TLSv1.2' | 'TLSv1.3')[], |
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 recommend modifying the original type definition.
| ADC_NAME: consumerGroup.name, | ||
| }, | ||
| name: undefined, | ||
| consumers: 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.
This is to allow recursiveOmitUndefined to remove them; please leave it as is.
First, the id, name, consumers expanded from consumerGroup will be overwritten with undefined, then subsequently removed by recursiveOmitUndefined—that's how they function.
Additionally, this modification is useless, as ADC does not support Consumer Group; thus, they are now essentially placeholders.
| "esnext" | ||
| ] | ||
| ], | ||
| "skipLibCheck": true |
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 this necessary?
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.
sure
Description
sdkdifferconverter-openapibackend-api7backend-apisix-standalonebackend-apisixcliChecklist