-
Notifications
You must be signed in to change notification settings - Fork 59
feat: Bump dependencies #401
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
Conversation
WalkthroughUpdated dependency versions across the repo and tooling. Small runtime changes: TypeORM query service selects aliased ids for distinct updateMany, paging logic now omits offset when fetching all with negative limit and zero offset, tests use Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant OffsetPager
participant QueryBuilder
Note over OffsetPager: New handling for enableFetchAllWithNegative
Client->>OffsetPager: createQuery(paging)
alt enableFetchAllWithNegative && paging.limit == -1
OffsetPager->>OffsetPager: delete paging.limit
opt paging.offset == 0
OffsetPager->>OffsetPager: delete paging.offset
end
end
OffsetPager->>QueryBuilder: build query with resulting paging
QueryBuilder-->>Client: return query/results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit 9fc783f
☁️ Nx Cloud last updated this comment at |
…ORMQueryService - Updated column alias in `TypeORMQueryService` for distinct records to fix query alignment. - Ensured consistency in record mapping by modifying the alias to "id AS id".
…st data setup - Updated `delete` to `deleteAll` for clearing repositories in test fixtures. - Ensured compatibility with updated TypeORM API.
- Downgraded `@typegoose/typegoose` to `12.11.0` for compatibility. - Added `@as-integrations/express5@^1.1.2` to dependencies. - Adjusted `mongoose` version to `8.10.0`. - Updated `yarn.lock` to reflect dependency changes.
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
package.json (1)
18-34: Dependency bumps are sound; only Mongoose needs extra attention.
- NestJS v11.1.6 and TypeORM 0.3.27 introduce no breaking changes.
- Mongoose 8.10.0 has known test/runtime hang issues—upgrade to 8.10.1 or later, or thoroughly test connection shutdowns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
package.json(3 hunks)packages/query-graphql/__tests__/integration/federation-n1/fixtures/test-data.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: check (23.x, test)
- GitHub Check: check (22.x, build)
- GitHub Check: check (23.x, lint)
- GitHub Check: check (23.x, build)
- GitHub Check: check (22.x, lint)
- GitHub Check: e2e-test (21.x, postgres)
- GitHub Check: e2e-test (21.x, mysql)
- GitHub Check: e2e-test (20.x, postgres)
- GitHub Check: check (22.x, test)
- GitHub Check: e2e-test (20.x, mysql)
- GitHub Check: Analyze
🔇 Additional comments (2)
packages/query-graphql/__tests__/integration/federation-n1/fixtures/test-data.ts (1)
9-10: LGTM! Proper migration to TypeORM's explicit API.The change from
delete({})todeleteAll()correctly adopts TypeORM 0.3.x's more explicit repository API, improving code clarity and aligning with the dependency bump.package.json (1)
39-117: Verify compatibility of upgraded devDependencies
- graphql 16.11.0: dropped positional args for graphql(), strips trailing newlines; update any affected call sites/tests.
- TypeScript ^5.9.3: patch only, but v5.9’s inference changes may surface new errors; run a full typecheck.
- @apollo/gateway & @apollo/subgraph 2.11.2: no breaking changes in these patches, but federation internals can change; validate supergraph composition and run integration/federation tests.
Run your test suite, schema composition, and integration smoke tests before rolling out to production.
…ategies - Added logic to remove `offset` when it equals zero to prevent unnecessary query parameters. - Ensured consistent behavior for `limit` and `offset` when `enableFetchAllWithNegative` is true.
- Updated `mongoose` dependency to version 8.10.1 in `package.json`. - Synchronized changes in `yarn.lock` to reflect the updated dependency.
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
examples/fetch-all-with-negative/e2e/todo-offset-fetch-all-enable.resolver.spec.ts (1)
15-15: Consider using strict equality.The
describeIfhelper uses loose equality (==). While this works for string comparisons, prefer strict equality (===) for consistency and to avoid potential type coercion issues.Apply this diff:
- const describeIf = (condition: boolean) => (condition ? describe : describe.skip) + const describeIf = (condition: boolean) => (condition ? describe : describe.skip)Actually, looking closer, the condition parameter is already typed as boolean, so the comparison happens at the call site. The real issue is in the usage at lines 67 and 95:
- describeIf(process.env.NESTJS_QUERY_DB_TYPE == 'postgres')('postgres', () => { + describeIf(process.env.NESTJS_QUERY_DB_TYPE === 'postgres')('postgres', () => {- describeIf(process.env.NESTJS_QUERY_DB_TYPE == 'mysql')('mysql', () => { + describeIf(process.env.NESTJS_QUERY_DB_TYPE === 'mysql')('mysql', () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
examples/fetch-all-with-negative/e2e/todo-offset-fetch-all-enable.resolver.spec.ts(2 hunks)package.json(3 hunks)packages/query-graphql/src/types/connection/cursor/pager/strategies/limit-offset.pager-strategy.ts(1 hunks)packages/query-graphql/src/types/connection/offset/pager/pager.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: check (22.x, build)
- GitHub Check: check (23.x, test)
- GitHub Check: check (22.x, lint)
- GitHub Check: check (23.x, build)
- GitHub Check: e2e-test (20.x, postgres)
- GitHub Check: e2e-test (21.x, postgres)
- GitHub Check: check (23.x, lint)
- GitHub Check: e2e-test (20.x, mysql)
- GitHub Check: check (22.x, test)
- GitHub Check: e2e-test (21.x, mysql)
- GitHub Check: Analyze
🔇 Additional comments (7)
packages/query-graphql/src/types/connection/cursor/pager/strategies/limit-offset.pager-strategy.ts (2)
33-33: Minor wording improvement in comment.The comment now says "if paging backwards" instead of the previous wording. This is clearer and more concise.
44-51: Verify downstream handling of omitted paging properties.
Automated search didn’t surface any uses ofpaging.limit/paging.offsetin query executors—please confirm that all adapters (TypeORM, Sequelize, Mongoose, etc.) gracefully handle undefinedlimit/offset(e.g. default to no limit) to avoid unexpected behavior.packages/query-graphql/src/types/connection/offset/pager/pager.ts (1)
73-80: Consistent paging refinement across strategies.This logic mirrors the changes in
limit-offset.pager-strategy.ts, ensuring both cursor-based and offset-based paging handle fetch-all scenarios consistently. The approach of omittingpaging.limitandpaging.offsetwhen appropriate creates a cleaner query shape.examples/fetch-all-with-negative/e2e/todo-offset-fetch-all-enable.resolver.spec.ts (2)
67-93: Database-specific test isolation looks good.The Postgres test correctly validates that fetch-all with offset works and returns the expected 98 items (100 total - 2 offset). The
hasPreviousPage: trueassertion is correct given the offset > 0.
95-116: MySQL limitation correctly tested.The test correctly validates that MySQL throws an error when attempting to use OFFSET without LIMIT, which is a known MySQL limitation. The error message check confirms the expected behavior.
package.json (2)
36-117: DevDependency updates look reasonable.Extensive devDependency updates including:
- Testing tools:
@types/supertest6.0.3,supertest7.1.4,ts-jest29.4.4- Build tools:
typescript5.9.3,ts-loader9.5.4- Linting:
eslint-plugin-import2.32.0,eslint-plugin-prettier5.5.4- Apollo:
@apollo/gateway2.11.2,@apollo/subgraph2.11.2- Docusaurus: 3.9.1 across the board
These are primarily patch and minor version updates, which should be low-risk.
18-34: No known security vulnerabilities in updated dependencies. All core packages (@nestjs/common, core, platform-express v11.1.6; @nestjs/graphql v13.2.0; mongoose 8.10.1; sequelize 6.37.7; typeorm 0.3.27) are on patched versions. Ensure compatibility across these updates and confirm that the e2e suite passes.
Summary by CodeRabbit