adborroto/PLAT 5 Convert to Typescript #367
Conversation
|
Plan Error |
WalkthroughThis pull request represents a comprehensive migration of a MongoDB pagination library from JavaScript to TypeScript. The changes span multiple files and involve several key transformations:
The migration aims to improve type safety, developer experience, and maintainability while preserving the library's core functionality. Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
🧹 Nitpick comments (19)
src/utils/resolveFields.ts (2)
Line range hint
50-60: Leverage TypeScript's Type System Instead of Runtime Type ChecksSince TypeScript provides static type checking, you can remove the runtime type checks using
Array.isArrayand_.isObject. This simplification relies on TypeScript to enforce the correct types at compile time.Apply this diff to remove the runtime checks:
-function resolveFields( - desiredFields?: string[] | null, - allowedFields?: Record<string, any> | null, - overrideFields?: Record<string, any> | null -) { - if (desiredFields != null && !Array.isArray(desiredFields)) { - throw new TypeError('expected nullable array for desiredFields'); - } - - if (allowedFields != null && !_.isObject(allowedFields)) { - throw new TypeError('expected nullable plain object for allowedFields'); - } - - if (overrideFields !== undefined && !_.isObject(overrideFields)) { - throw new TypeError('expected optional plain object for overrideFields'); - } +function resolveFields( + desiredFields?: string[] | null, + allowedFields?: Record<string, any> | null, + overrideFields?: Record<string, any> | null +) {
Line range hint
19-33: Replace_.reducewith Native JavaScriptObject.entriesTo reduce dependencies and improve performance, consider replacing
_.reducewithObject.entriesand the nativeArray.prototype.reducemethod.Apply this diff to make the change:
-const fields = _.reduce( - projection, - (memo, value, key) => { +const fields = Object.entries(projection).reduce( + (memo, [key, value]) => { if (key !== '_id' && value !== undefined && !value) { throw new TypeError('projection includes exclusion, but we do not support that'); } if (value || (key === '_id' && value === undefined && includeIdDefault)) { memo.push(key); } return memo; }, [] );src/config.ts (1)
15-15: Consider using a more specific type for COLLATIONUsing
Record<string, any>reduces TypeScript's type safety benefits. Consider defining a more specific interface for the collation structure if possible.- COLLATION: Record<string, any>; + COLLATION: { + locale?: string; + caseLevel?: boolean; + strength?: number; + // Add other specific collation options + } | null;src/index.ts (1)
1-9: Consider organizing imports by categoryGroup related imports together for better maintainability:
- Core functionality
- Plugins
- Utils
+// Core functionality import aggregate from './aggregate'; import config from './config'; import find from './find'; import findWithReq from './findWithReq'; -import mongoosePlugin from './mongoose.plugin'; import search from './search'; + +// Plugins +import mongoosePlugin from './mongoose.plugin'; + +// Utils import { encodePaginationTokens } from './utils/query'; import sanitizeQuery from './utils/sanitizeQuery';src/utils/getPropertyViaDotNotation.ts (2)
15-15: Avoid usinganytypeUsing
anybypasses TypeScript's type checking. Consider using a more specific type.- let prop: any = object; + let prop: unknown = object;
9-12: Consider improving return type specificityThe current return type
unknowncould be more specific based on the input typeT.-): unknown { +): T[keyof T] | undefined {test/support/db.ts (2)
11-11: Type the environment variableConsider using a const assertion or type guard for the driver version.
-const driver_version = process.env.DRIVER_VERSION; +const driver_version = process.env.DRIVER_VERSION as 'v3' | undefined;
18-18: Type the driver parameter more strictlyThe
driverparameter could have a more specific type.-async function db(mongod: MongoMemoryServer, driver: string | null = null): Promise<DbResponse> { +async function db(mongod: MongoMemoryServer, driver: 'mongoist' | null = null): Promise<DbResponse> {src/utils/bsonUrlEncoding.ts (1)
8-8: Consider using a more specific type forBSON_UNDEFINEDThe
as constassertion is good, but the type could be more specific to prevent potential conflicts.-const BSON_UNDEFINED = '__mixmax__undefined__' as const; +const BSON_UNDEFINED = '__mixmax__undefined__' as '__mixmax__undefined__'; -export type Encodable = Record<string, unknown> | unknown[] | null | string | number | boolean; +export type Encodable = Record<string, unknown> | unknown[] | null | string | number | boolean | undefined;Also applies to: 13-13
src/findWithReq.ts (1)
23-27: Consider adding request query type safetyThe function could benefit from a more specific type for the expected query parameters.
+interface FindRequestQuery { + limit?: string; + next?: string; + previous?: string; + fields?: string; +} export default async function findWithReq( - req: Request, + req: Request<any, any, any, FindRequestQuery>, collection: Collection<Document>, params: FindParams ): Promise<PaginationResponse<Document>> {test/utils/bsonUrlEncoding.test.ts (1)
11-13: Use interface for test contextDefine an interface for the test context object to improve type safety.
+interface TestContext { + db: typeof mongo | null; +} - const t = { - db: null, - }; + const t: TestContext = { + db: null, + };src/mongoose.plugin.ts (1)
29-29: Avoid mutating input parametersThe functions modify the input parameters directly. Consider creating a copy instead.
- params = _.extend({}, params); + const extendedParams = _.extend({}, params); - return find(this.collection, params); + return find(this.collection, extendedParams); - params = _.extend({}, params); + const extendedParams = _.extend({}, params); - return search(this.collection, searchString, params); + return search(this.collection, searchString, extendedParams);Also applies to: 49-49
test/utils/query.test.ts (2)
67-67: Improve test readability with meaningful test valuesThe test values
'1'and'2'in the expected query are not representative of real data and make it hard to understand the test's intent.Consider using more meaningful test values that reflect actual use cases:
- $or: [{ name: { $lt: '1' } }, { name: null }, { _id: { $lt: '2' }, name: { $eq: '1' } }], + $or: [{ name: { $lt: 'Test' } }, { name: null }, { _id: { $lt: '123' }, name: { $eq: 'Test' } }],
51-70: Add more test cases for edge casesThe current test suite for
generateCursorQueryonly covers basic scenarios. Consider adding tests for:
- Invalid cursor values
- Empty string values
- Special characters in field names
src/utils/sanitizeQuery.ts (2)
5-11: Consider stricter typing for the index signatureThe current index signature
[key: string]: anyallows any value type, which reduces TypeScript's type safety benefits.Consider using a more specific type or union type for the index signature value:
- [key: string]: any; + [key: string]: string | string[] | undefined;
Line range hint
53-58: Add type guard for limit parsingThe limit parsing logic could be made more type-safe with a proper type guard.
Consider adding a type guard:
- if (!_.isEmpty(query.limit)) { + if (typeof query.limit === 'string' && query.limit.length > 0) { const limit = parseInt(query.limit, 10); // Don't let the user specify a higher limit than params.limit, if defined. if (!isNaN(limit) && (!params.limit || params.limit > limit)) { params.limit = limit; } }test/search.test.ts (1)
9-12: Add type definition for test context objectConsider defining an interface for the test context object to improve type safety.
interface TestContext { db: typeof import('mongodb').Db | null; } const t: TestContext = { db: null, };README.md (2)
33-36: Enhance TypeScript migration documentation.While the new section announces the TypeScript conversion, consider adding:
- Breaking changes (if any)
- Migration guide for existing JavaScript users
- Examples of using TypeScript types and interfaces
- Location of type definitions for TypeScript users
33-36: Add TypeScript code examples.Consider updating the code examples in the documentation to TypeScript to:
- Show proper type annotations
- Demonstrate type safety benefits
- Help users understand how to use the library with TypeScript
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (43)
.babelrc(0 hunks).gitignore(1 hunks).nvmrc(1 hunks)README.md(1 hunks)jest.config.js(1 hunks)package.json(4 hunks)src/aggregate.js(0 hunks)src/aggregate.ts(1 hunks)src/config.js(0 hunks)src/config.ts(1 hunks)src/find.js(0 hunks)src/find.ts(1 hunks)src/findWithReq.js(0 hunks)src/findWithReq.ts(1 hunks)src/index.js(0 hunks)src/index.ts(1 hunks)src/mongoose.plugin.js(0 hunks)src/mongoose.plugin.ts(1 hunks)src/search.js(0 hunks)src/search.ts(1 hunks)src/utils/bsonUrlEncoding.js(0 hunks)src/utils/bsonUrlEncoding.ts(1 hunks)src/utils/getPropertyViaDotNotation.js(0 hunks)src/utils/getPropertyViaDotNotation.ts(1 hunks)src/utils/query.js(0 hunks)src/utils/query.ts(1 hunks)src/utils/resolveFields.ts(3 hunks)src/utils/sanitizeParams.ts(4 hunks)src/utils/sanitizeQuery.ts(3 hunks)test/.eslintrc.json(0 hunks)test/aggregate.test.ts(1 hunks)test/find.test.ts(1 hunks)test/findWithReq.test.ts(18 hunks)test/mongoosePlugin.test.ts(4 hunks)test/search.test.ts(1 hunks)test/support/db.js(0 hunks)test/support/db.ts(1 hunks)test/utils/bsonUrlEncoding.test.ts(2 hunks)test/utils/query.test.js(0 hunks)test/utils/query.test.ts(1 hunks)test/utils/resolveFields.test.ts(1 hunks)tsconfig-lint.json(1 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (14)
- .babelrc
- test/.eslintrc.json
- src/aggregate.js
- src/config.js
- src/find.js
- src/index.js
- src/utils/getPropertyViaDotNotation.js
- test/utils/query.test.js
- src/utils/bsonUrlEncoding.js
- src/search.js
- src/mongoose.plugin.js
- test/support/db.js
- src/utils/query.js
- src/findWithReq.js
✅ Files skipped from review due to trivial changes (5)
- .nvmrc
- .gitignore
- tsconfig-lint.json
- test/utils/resolveFields.test.ts
- tsconfig.json
🧰 Additional context used
🪛 Biome (1.9.4)
test/mongoosePlugin.test.ts
[error] 70-70: Do not shadow the global "hasOwnProperty" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
[error] 91-91: Do not shadow the global "hasOwnProperty" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (9)
jest.config.js (1)
9-9: Verify Impact of RemovingmoduleNameMapperConfigurationThe
moduleNameMappersection, which previously handled module path mappings, has been removed. If any tests depended on this mapping, it could lead to module resolution errors.Please ensure that removing
moduleNameMapperdoes not affect the ability of Jest to resolve modules, especially if there are custom module paths.test/search.test.ts (1)
1-4: LGTM! Clean ES module importsThe conversion from CommonJS to ES module imports is done correctly, with proper TypeScript type imports.
test/aggregate.test.ts (2)
1-4: LGTM! Consistent import patternThe import statements follow the same pattern as other test files, maintaining consistency across the codebase.
10-12: Maintain consistent type definitions across test filesFor consistency, use the same TestContext interface suggested for search.test.ts.
test/find.test.ts (2)
10-12: Maintain consistent type definitions across test filesFor consistency, use the same TestContext interface suggested for search.test.ts.
4-4: Verify the impact of switching ObjectId import sourceThe change from
mongoisttomongodbfor ObjectId import is correct for TypeScript, but verify that it maintains the same functionality.✅ Verification successful
Import change from mongoist to mongodb ObjectId is correct
The ObjectId is only used to generate string IDs via toString(), which is standard functionality supported by both packages. The change is safe and maintains the same behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining mongoist ObjectId usage rg "require.*mongoist.*ObjectId" || rg "from.*mongoist.*ObjectId" # Check if mongodb ObjectId is used consistently rg "ObjectId.*mongodb" -A 2Length of output: 222
Script:
#!/bin/bash # Check how ObjectId is used in the test file rg "ObjectId" test/find.test.ts -A 2Length of output: 1032
package.json (3)
14-20: LGTM! Build and test scripts are properly configured for TypeScript.The scripts section has been correctly updated to support TypeScript:
- Lint script includes TypeScript type checking
- Build script properly cleans and compiles TypeScript
- Test scripts are well-organized for different drivers
55-59: LGTM! TypeScript tooling is properly configured.All necessary TypeScript development dependencies are present with appropriate versions:
- TypeScript compiler and configuration
- ESLint TypeScript plugins
- ts-jest for TypeScript testing
Also applies to: 71-73
42-42: 🛠️ Refactor suggestionConsider using a stable version of @types/express.
The current version
^5.0.0is a pre-release version which could introduce instability. Consider using the stable version^4.17.21instead.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (15)
src/utils/bsonUrlEncoding.ts (2)
32-34:⚠️ Potential issueHandle all
undefinedvalues in arraysThe current implementation replaces
undefinedonly at the first index of arrays. This could missundefinedvalues at other positions, leading to potential encoding issues.Apply this diff to replace all occurrences of
undefinedin arrays:- if (Array.isArray(obj) && obj[0] === undefined) { - obj[0] = BSON_UNDEFINED; + if (Array.isArray(obj)) { + for (let i = 0; i < obj.length; i++) { + if (obj[i] === undefined) { + obj[i] = BSON_UNDEFINED; + } + } }
47-49:⚠️ Potential issueHandle all
BSON_UNDEFINEDvalues in arrays during decodingSimilar to the
encodemethod, thedecodemethod only checks the first element of arrays forBSON_UNDEFINED. This could missBSON_UNDEFINEDvalues at other positions.Apply this diff to replace all occurrences of
BSON_UNDEFINEDin arrays:- if (Array.isArray(obj) && obj[0] === BSON_UNDEFINED) { - obj[0] = undefined; + if (Array.isArray(obj)) { + for (let i = 0; i < obj.length; i++) { + if (obj[i] === BSON_UNDEFINED) { + obj[i] = undefined; + } + } }src/aggregate.ts (1)
46-46:⚠️ Potential issueFix potential error when spreading undefined options
If
params.optionsisundefined, spreading it will cause aTypeError. To prevent this, defaultparams.optionsto an empty object before spreading.Apply this diff to fix the issue:
- const options: Record<string, any> = { ...params.options }; + const options: Record<string, any> = { ...(params.options || {}) };src/search.ts (3)
6-11:⚠️ Potential issueUpdate
SearchParams.nexttype to handle decoded cursor correctlyThe
nextparameter inSearchParamsis currently typed asstring, but after decoding, it becomes a tuple[number, any]. This mismatch can lead to type errors and runtime issues when accessingparams.next[0]andparams.next[1].Apply this diff to update the
SearchParamsinterface:export interface SearchParams { query?: Record<string, any>; limit?: number; fields?: Record<string, number>; - next?: string; + next?: string | [number, any]; }
38-39:⚠️ Potential issueCorrect the type casting of
params.nextafter decodingAfter decoding
params.next, it should be cast to[number, any]instead ofstringto reflect its actual structure and ensure type safety when accessing its elements.Apply this diff to correct the casting:
if (params.next) { - params.next = bsonUrlEncoding.decode(params.next) as string; + params.next = bsonUrlEncoding.decode(params.next) as [number, any]; }
57-61:⚠️ Potential issuePrevent
params.fieldsfrom overriding protected fields in$projectstageDirectly spreading
params.fieldsinto the$projectstage can allow overriding of critical fields like_idandscore, which may lead to security risks or unintended behavior. To safeguard these fields, exclude them fromparams.fieldsbefore spreading.Apply this diff to fix the issue:
$project: { - ...params.fields, _id: 1, score: { $meta: 'textScore' }, + ..._.omit(params.fields, ['_id', 'score']), },src/find.ts (2)
14-20:⚠️ Potential issueInclude
hintproperty inFindParamsinterfaceThe
hintproperty is used in the code (line 75) but is not defined in theFindParamsinterface. This can lead to TypeScript type errors and may cause issues during compilation.Apply this diff to include the
hintproperty:export interface FindParams extends PaginationParams { query?: Document; limit?: number; fields?: Record<string, number>; collation?: Record<string, any> | null; overrideFields?: Record<string, number>; + hint?: any; }
37-42:⚠️ Potential issueAwait the asynchronous
aggregatefunction callThe
aggregatefunction is asynchronous but is not awaited. This meansresponsewill be aPromiseinstead of the expected result, which can lead to unexpected behavior when accessingresponse.Apply this diff to fix the issue:
response = aggregate( + response = await aggregate( collection, Object.assign({}, params, { aggregation: params.query ? [{ $match: params.query }] : [], }) );test/support/db.ts (1)
25-30:⚠️ Potential issueImprove error handling and type safety in client connection
The client connection logic needs better error handling and type assertions.
- const clientToConnect = driver_version === 'v3' ? MongoClientV3 : MongoClient; - const [client, dbName] = await Promise.all([clientToConnect.connect(uri), mongod.getDbName()]); - return { - db: client.db(dbName), - client, - }; + const clientToConnect = driver_version === 'v3' ? MongoClientV3 : MongoClient; + try { + const [client, dbName] = await Promise.all([ + clientToConnect.connect(uri), + mongod.getDbName(), + ]) as [MongoClient | MongoClientV3, string]; + return { + db: client.db(dbName), + client, + }; + } catch (error) { + throw new Error(`Failed to connect to MongoDB: ${error instanceof Error ? error.message : 'Unknown error'}`); + }src/findWithReq.ts (1)
28-32:⚠️ Potential issueAdd input validation for query parameters
The function should validate query parameters before processing.
+ // Validate query parameters before sanitizing + if (req.query.limit && isNaN(Number(req.query.limit))) { + throw new Error('Invalid limit parameter: must be a number'); + } + // Sanitize the query string parameters and merge with additional params const sanitizedParams = sanitizeQuery(req.query, params); // Perform the find operation with the sanitized parameters return find(collection, sanitizedParams);test/utils/bsonUrlEncoding.test.ts (1)
39-39: 🛠️ Refactor suggestionReplace
anytype with a proper interfaceUsing
anydefeats TypeScript's type checking benefits.- const decoded: any = bsonUrlEncoding.decode(str); + interface DecodedObject { + _id: mongo.ObjectID; + date: Date; + number: number; + string: string; + } + const decoded = bsonUrlEncoding.decode(str) as DecodedObject;src/mongoose.plugin.ts (2)
24-31: 🛠️ Refactor suggestionImprove type safety in findFn implementation
The function should use proper typing for the Mongoose model context and return value.
- const findFn = async function(this: any, params: FindParams): Promise<any> { + const findFn = async function<T>(this: Model<T>, params: FindParams): Promise<PaginationResponse<T>> { if (!this.collection) { throw new Error('collection property not found'); } params = _.extend({}, params); return find(this.collection, params); };
40-51: 🛠️ Refactor suggestionImprove type safety in searchFn implementation
The search function needs proper typing for the
thiscontext and return type.const searchFn = async function( - this: any, + this: Model<T>, searchString: string, params: SearchParams - ): Promise<any> { + ): Promise<PaginationResponse<T>> { if (!this.collection) { throw new Error('collection property not found'); } params = _.extend({}, params); return search(this.collection, searchString, params); };test/mongoosePlugin.test.ts (2)
1-3: 🛠️ Refactor suggestionProper type definitions needed for plugin methods
While the ES module imports are correct, the file needs proper type definitions for the plugin methods to avoid
as anycasts throughout the tests.Consider adding type definitions as suggested in the previous review:
declare module 'mongoose' { interface Model<T> { paginate(): Promise<PaginationResult<T>>; paginateFN(): Promise<PaginationResult<T>>; search(query: string, options?: SearchOptions): Promise<SearchResult<T>>; searchFN(query: string, options?: SearchOptions): Promise<SearchResult<T>>; } }
70-70:⚠️ Potential issueFix shadowing of global hasOwnProperty
The variable
hasOwnPropertyshadows the global property of the same name.Apply this fix:
- var hasOwnProperty = Object.prototype.hasOwnProperty; + const hasOwn = Object.prototype.hasOwnProperty; - expect(hasOwnProperty.call(data, 'results')).toBe(true); + expect(hasOwn.call(data, 'results')).toBe(true);Also applies to: 91-91
🧰 Tools
🪛 Biome (1.9.4)
[error] 70-70: Do not shadow the global "hasOwnProperty" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🧹 Nitpick comments (10)
src/utils/sanitizeQuery.ts (1)
52-52: Remove redundant assignment ofparamsThe variable
paramsalready has a default value{}in the function signature. The assignmentparams = params || {};is unnecessary and can be removed for cleaner code.Apply this diff to remove the redundant line:
- params = params || {};src/search.ts (1)
46-47: Remove unnecessary non-null assertions onparams.limitThe non-null assertion operator
!is unnecessary here becauseparams.limithas been defaulted earlier. Removing!makes the code cleaner and avoids potential confusion.Apply this diff to simplify the code:
- if (params.limit! < 1) params.limit = 1; - if (params.limit! > config.MAX_LIMIT) params.limit = config.MAX_LIMIT; + if (params.limit < 1) params.limit = 1; + if (params.limit > config.MAX_LIMIT) params.limit = config.MAX_LIMIT;jest.config.js (3)
3-5: Simplify Configuration by Removing RedundanttransformPropertyWhen using the
ts-jestpreset, thetransformconfiguration is set automatically. You can remove thetransformproperty to simplify the configuration.Apply this diff:
module.exports = { preset: 'ts-jest', - transform: { - '^.+\\.tsx?$': 'ts-jest', - }, moduleFileExtensions: ['ts', 'js', 'json', 'node'],
6-6: Include'tsx'inmoduleFileExtensionsIf your project includes
.tsxfiles (e.g., React components), consider adding'tsx'to themoduleFileExtensionsarray to ensure Jest recognizes these files.Apply this diff:
moduleFileExtensions: ['ts', 'js', 'json', 'node'], +moduleFileExtensions: ['ts', 'tsx', 'js', 'json', 'node'],
8-8: UpdatecollectCoverageFromto Include.tsxFilesTo include coverage data for
.tsxfiles, update thecollectCoverageFrompattern.Apply this diff:
-collectCoverageFrom: ['src/**/*.ts'], +collectCoverageFrom: ['src/**/*.{ts,tsx}'],test/support/db.ts (1)
11-11: Consider using a typed environment variableThe
driver_versionvariable could benefit from type safety using a union type of allowed values.-const driver_version = process.env.DRIVER_VERSION; +const driver_version = process.env.DRIVER_VERSION as 'v3' | undefined;test/find.test.ts (1)
10-12: Consider adding type annotations to the test contextWhile the test setup works, it would be better to add proper TypeScript types to the test context object.
- const t = { - db: null, - }; + interface TestContext { + db: any; // or proper MongoDB type + } + const t: TestContext = { + db: null, + };tsconfig-lint.json (2)
2-5: Consider adding strict type checking optionsWhile extending the base config is good, consider explicitly enabling strict type checking options for better type safety.
{ "extends": "@mixmaxhq/ts-config/tsconfig.json", "compilerOptions": { - "outDir": "dist/node" + "outDir": "dist/node", + "strict": true, + "noImplicitAny": true, + "strictNullChecks": true },
6-13: Consider adding more specific exclude patternsThe current exclude patterns are good, but consider adding more specific patterns to exclude test fixtures and generated files.
"exclude": [ "node_modules", - "dist" + "dist", + "coverage", + "**/*.spec.ts", + "**/*.mock.ts", + "**/__fixtures__/*" ]test/utils/resolveFields.test.ts (1)
Line range hint
1-74: Consider enhancing tests with TypeScript features.While the migration is correct, consider enhancing the tests with TypeScript features:
- Add type annotations for test inputs and expected outputs
- Use TypeScript's
typeorinterfacefor complex test data structures- Leverage TypeScript's strict type checking in assertions
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (43)
.babelrc(0 hunks).gitignore(1 hunks).nvmrc(1 hunks)README.md(1 hunks)jest.config.js(1 hunks)package.json(4 hunks)src/aggregate.js(0 hunks)src/aggregate.ts(1 hunks)src/config.js(0 hunks)src/config.ts(1 hunks)src/find.js(0 hunks)src/find.ts(1 hunks)src/findWithReq.js(0 hunks)src/findWithReq.ts(1 hunks)src/index.js(0 hunks)src/index.ts(1 hunks)src/mongoose.plugin.js(0 hunks)src/mongoose.plugin.ts(1 hunks)src/search.js(0 hunks)src/search.ts(1 hunks)src/utils/bsonUrlEncoding.js(0 hunks)src/utils/bsonUrlEncoding.ts(1 hunks)src/utils/getPropertyViaDotNotation.js(0 hunks)src/utils/getPropertyViaDotNotation.ts(1 hunks)src/utils/query.js(0 hunks)src/utils/query.ts(1 hunks)src/utils/resolveFields.ts(3 hunks)src/utils/sanitizeParams.ts(4 hunks)src/utils/sanitizeQuery.ts(3 hunks)test/.eslintrc.json(0 hunks)test/aggregate.test.ts(1 hunks)test/find.test.ts(1 hunks)test/findWithReq.test.ts(18 hunks)test/mongoosePlugin.test.ts(4 hunks)test/search.test.ts(1 hunks)test/support/db.js(0 hunks)test/support/db.ts(1 hunks)test/utils/bsonUrlEncoding.test.ts(2 hunks)test/utils/query.test.js(0 hunks)test/utils/query.test.ts(1 hunks)test/utils/resolveFields.test.ts(1 hunks)tsconfig-lint.json(1 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (14)
- test/.eslintrc.json
- .babelrc
- src/utils/bsonUrlEncoding.js
- src/utils/getPropertyViaDotNotation.js
- src/config.js
- src/find.js
- src/aggregate.js
- src/search.js
- src/index.js
- test/support/db.js
- src/mongoose.plugin.js
- src/utils/query.js
- src/findWithReq.js
- test/utils/query.test.js
🧰 Additional context used
🪛 Biome (1.9.4)
test/mongoosePlugin.test.ts
[error] 70-70: Do not shadow the global "hasOwnProperty" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
[error] 91-91: Do not shadow the global "hasOwnProperty" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (31)
src/search.ts (1)
86-87: Verify the existence ofaggregateAsCursormethod on theCollectiontypeThe check
'aggregateAsCursor' in collectionassumes that thecollectionobject may have anaggregateAsCursormethod. Please verify that this method exists on theCollection<T>type provided by the MongoDB driver. If it does not, this could lead to a runtime error.Since my knowledge cutoff is in 2023-10, please confirm whether
aggregateAsCursoris a valid method. If it's a custom implementation or from a newer version of the driver, ensure it is correctly typed and handled.src/utils/resolveFields.ts (2)
13-15: Refine type definitions forprojectionparameterUsing
Record<string, any>for theprojectionparameter reduces type safety, as it allows any values. Consider defining a more specific type to leverage TypeScript's type system effectively.You can define a type that specifies the allowed values for
projection. For example, ifprojectionshould map field names to booleans:type Projection = Record<string, boolean>; function fieldsFromMongo( projection: Projection = {}, includeIdDefault = false ): ProjectionFieldSet { // function body }
47-49: Improve type safety forallowedFieldsandoverrideFieldsparametersCurrently,
allowedFieldsandoverrideFieldsare typed asRecord<string, any>, which can reduce type safety. Defining more precise types can help catch errors at compile time and make the code more robust.Consider specifying the expected value types. For example:
type FieldsMap = Record<string, boolean>; function resolveFields( desiredFields?: string[] | null, allowedFields?: FieldsMap | null, overrideFields?: FieldsMap | null ): Record<string, any> | null { // function body }src/utils/query.ts (4)
45-45: Handle UndefinedpaginatedFieldWhen Accessing Object PropertiesSince
params.paginatedFieldis optional, accessing it without checking may lead to runtime errors. Ensure thatpaginatedFieldis defined before using it withobjectPath.get.Consider adding a validation check:
export function encodePaginationTokens<T>( params: PaginationParams, response: PaginationResponse<T>, previous: T | null, next: T | null ): void { + if (!params.paginatedField) { + throw new Error('`paginatedField` is required for pagination tokens'); + } const shouldSecondarySortOnId = params.paginatedField !== '_id';
55-55: Handle UndefinedpaginatedFieldWhen Accessing Object PropertiesAs with the previous occurrence, ensure that
params.paginatedFieldis defined before using it withobjectPath.getto prevent runtime errors.
107-107: Default Optional Boolean ParametersortAscending
params.sortAscendingis optional and may beundefined, which can cause logical inconsistencies. Default this parameter to a boolean value to ensure consistent behavior in sorting.Apply this diff to set a default value:
export function generateSort(params: PaginationParams): SortObject { + const sortAscending = params.sortAscending ?? true; const sortAsc = - (!params.sortAscending && params.previous) || (params.sortAscending && !params.previous); + (!sortAscending && params.previous) || (sortAscending && !params.previous);
124-214: ValidatepaginatedFieldingenerateCursorQueryFunctionThe
generateCursorQueryfunction assumesparams.paginatedFieldis defined. If it'sundefined, it may lead to incorrect queries or runtime errors. ValidatepaginatedFieldbefore proceeding.Consider adding a validation check:
export function generateCursorQuery(params: PaginationParams): CursorQuery { + if (!params.paginatedField) { + throw new Error('`paginatedField` is required to generate cursor query'); + } if (!params.next && !params.previous) return {}; const sortAsc = (!params.sortAscending && params.previous) || (params.sortAscending && !params.previous);src/utils/getPropertyViaDotNotation.ts (1)
13-13: Add Validation forpropertyNameParameterThe function should validate that
propertyNameis a non-empty string to prevent unexpected errors when splitting or accessing properties.Apply this diff:
export default function getPropertyViaDotNotation<T extends Record<string, any>>( propertyName: string, object: T ): unknown { + if (!propertyName || typeof propertyName !== 'string') { + throw new Error('`propertyName` must be a non-empty string'); + } const parts = propertyName.split('.');test/utils/query.test.ts (5)
8-28: LGTM! Well-structured test suite with proper type safety.The test suite effectively uses TypeScript generics and type annotations to ensure type safety. The
PaginatedTypegeneric constraint andPaginationResponsetype are used appropriately.
67-67: Verify the hardcoded test values.The test uses hardcoded values
'1'and'2'in the cursor query assertion. These values seem arbitrary and their significance is not clear from the test context.Consider either:
- Adding comments to explain the significance of these values
- Using more meaningful test data that reflects real-world scenarios
17-17: LGTM! Correct usage of mongoose Schema.Types.ObjectId.The type has been properly updated to use
mongoose.Schema.Types.ObjectIdwhich is the correct type for ObjectId references.
70-70: Fix shadowing of global hasOwnProperty.The variable
hasOwnPropertyshadows the global property of the same name.Apply this fix:
- var hasOwnProperty = Object.prototype.hasOwnProperty; + const hasOwn = Object.prototype.hasOwnProperty; - expect(hasOwnProperty.call(data, 'results')).toBe(true); + expect(hasOwn.call(data, 'results')).toBe(true);Also applies to: 91-91
59-59: Avoid usingas anytype assertions.Multiple instances of
as anytype assertions reduce TypeScript's type safety benefits.Consider creating proper type definitions for the mongoose plugin:
declare module 'mongoose' { interface Model<T> { paginate(): Promise<PaginationResult<T>>; paginateFN(): Promise<PaginationResult<T>>; search(query: string, options?: SearchOptions): Promise<SearchResult<T>>; searchFN(query: string, options?: SearchOptions): Promise<SearchResult<T>>; } }Also applies to: 64-64, 69-69, 80-80, 85-85, 90-90
test/search.test.ts (1)
1-11: LGTM! Clean conversion to TypeScript with improved type safety.The changes effectively:
- Convert to ES module imports
- Add explicit db property initialization
- Maintain test structure and readability
test/findWithReq.test.ts (1)
73-78: Improve type safety by creating proper request mock objects.Multiple instances of type assertions with
as anyandas Requestreduce type safety.Consider creating properly typed mock objects:
interface MockQuery { limit?: string; fields?: string; next?: string; previous?: string; } function createMockRequest(query: MockQuery): Request { return { query, } as unknown as Request; } // Usage: const request = createMockRequest({ limit: '2', fields: 'counter,myfield1', });Also applies to: 96-104, 120-121, 145-146, 168-169, 187-188, 207-208, 228-229, 252-253, 281-282, 305-306, 333-334, 360-361, 392-393, 421-422, 444-445, 477-478
test/find.test.ts (1)
1-4: Import statements look good but consider organizing themThe conversion to ES module syntax is correct. The imports are properly organized with external dependencies first, followed by internal imports.
.nvmrc (1)
1-1: Verify Node.js version compatibilityThe update to Node.js 18.20.0 is good, as it's a stable LTS version with good TypeScript support. However, please verify that all dependencies are compatible with this version.
✅ Verification successful
Node.js 18.20.0 is compatible with all dependencies
All package dependencies have version requirements that are satisfied by Node.js 18.20.0. The highest minimum requirement found is Node.js 18, and there are no maximum version constraints that would conflict with this version.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any package.json engines field restrictions if [ -f "package.json" ]; then echo "Checking engines field in package.json..." jq '.engines' package.json fi # Check for any dependency version conflicts if [ -f "package-lock.json" ]; then echo "Checking for any dependency version conflicts..." jq '.packages | to_entries[] | select(.value.engines != null) | .value.engines' package-lock.json fiLength of output: 26851
tsconfig.json (1)
9-13: Verify test file exclusion patternThe current pattern excludes all test files, but verify if you need to keep any specific test utilities or types.
✅ Verification successful
Test file exclusion pattern is correct
The current exclusion pattern is appropriate. The discovered test utility (test/support/db.ts) is specifically for test infrastructure and should be compiled separately with test-specific configuration, not with the main source code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any test utility files that might need to be included echo "Checking for test utility files that might need inclusion..." fd -e ts -E "*.test.ts" -E "*.spec.ts" . test/Length of output: 198
Script:
#!/bin/bash # Check if db.ts is imported in source files ast-grep --pattern 'import $_ from "$_/test/support/db"' # Also check relative imports that might resolve to this file ast-grep --pattern 'import $_ from "$_/db"' # Check actual content of db.ts to understand its purpose cat test/support/db.tsLength of output: 1044
src/config.ts (2)
1-16: Well-structured interface with clear documentation.The interface definition is clear and well-documented with JSDoc comments.
15-15: Type mismatch between interface and implementation.The
COLLATIONproperty is defined asRecord<string, any>but assignednull.Also applies to: 21-21
src/index.ts (1)
1-19: Clean and well-organized module exports.The file demonstrates good module organization with clear imports and a structured export block.
.gitignore (1)
43-43: Verify broader dist/ ignore pattern.The change from
dist/nodetodist/will ignore all compiled output. Please ensure this won't accidentally exclude any required distribution files.✅ Verification successful
Broader dist/ ignore pattern is correct
The change from
dist/nodetodist/is appropriate as:
- All build output is generated in dist/ directory
- dist/ is properly included in the published npm package
- No other important files are stored in dist/
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any potentially important files in dist/ that might be accidentally ignored fd . dist/ --type f --hidden --no-ignoreLength of output: 135
Script:
#!/bin/bash # Check configuration files and docs for dist references echo "=== Package.json ===" fd package.json --type f --exec cat {} echo -e "\n=== TypeScript Config ===" fd tsconfig.json --type f --exec cat {} echo -e "\n=== Build References ===" rg -i "(\bdist\b|outDir|build|output)" -g "!.git" -g "!*.lock" -g "!dist/*" -g "!{package,tsconfig}.json" echo -e "\n=== README Build Instructions ===" fd "README" --type f --exec cat {}Length of output: 18019
test/utils/resolveFields.test.ts (1)
1-1: Clean migration to ES module syntax.The import statement has been properly converted to use ES module syntax.
package.json (1)
14-20: Well-structured TypeScript build and test scripts.The scripts have been properly updated to support TypeScript:
- Added TypeScript compilation check to lint script
- Added clean build step with
tsc- Maintained separate test configurations for different drivers
src/utils/sanitizeParams.ts (3)
1-16: LGTM! Well-structured interface definition.The
SanitizeParamsinterface is well-defined with appropriate types for MongoDB pagination parameters.
18-21: Use proper MongoDB types for better type safety.The
collectionparameter is typed asany, which reduces type safety.
55-55: Add type guards for string operations.Type assertions are used without first checking if the value is actually a string.
Also applies to: 80-80
README.md (1)
33-36: LGTM! Clear documentation update.The new section effectively communicates the transition to TypeScript and its benefits.
test/aggregate.test.ts (2)
1-3: LGTM! Clean import syntax.The conversion to ES module syntax is correct and consistent.
10-12: LGTM! Type-safe initialization.The object initialization with explicit null assignment improves type safety.
test/mongoosePlugin.test.ts (1)
17-17: LGTM! Improved type clarityGood change using the explicit
mongoose.Schema.Types.ObjectIdinstead of the deprecatedmongoose.Schema.ObjectId.
Signed-off-by: Alejandro Dominguez <adborroto90@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
package.json (2)
Line range hint
75-77: Update Node.js engine requirement.The current engine requirement (
">= 6.9.1") is outdated for TypeScript support. Consider updating to match the TypeScript version's requirements."engines": { - "node": ">= 6.9.1" + "node": ">= 14.17.0" },
Line range hint
6-8: Update files field for TypeScript declaration files.Ensure TypeScript declaration files are included in the published package.
"files": [ "index.js", "dist", - "src" + "src", + "index.d.ts" ],
🧹 Nitpick comments (6)
tsconfig.json (1)
14-18: Consider separate test files configuration.Since test files are excluded, consider creating a separate
tsconfig.test.jsonfor test files to ensure proper type checking during testing.// tsconfig.test.json { "extends": "./tsconfig.json", "include": [ "test/**/*" ], "exclude": [ "node_modules", "dist" ] }package.json (1)
14-14: Add type checking to the build script.Consider adding type checking to the build process to catch type errors before compilation.
- "lint": "eslint . && tsc --noEmit", + "lint": "eslint . && npm run type-check", + "type-check": "tsc --noEmit", - "build": "rm -rf dist/ && tsc" + "build": "rm -rf dist/ && npm run type-check && tsc"Also applies to: 20-20
test/utils/sanitizeParams.test.ts (2)
15-23: Consider adding more edge cases for parameter decoding.While the basic decoding test is good, consider adding tests for:
- Invalid encoded values
- Empty arrays
- Null/undefined values
60-77: Enhance test coverage for case-insensitive sorting.The test only verifies the happy path. Consider adding tests for:
- Multiple fields with case-insensitive sorting
- Special characters in field values
- Empty field values
src/aggregate.ts (1)
12-17: Consider makingaggregationtype more specific.The
Document[]type foraggregationis too broad. Consider creating a union type of specific aggregation stages for better type safety.type AggregationStage = | { $match: Document } | { $sort: Document } | { $limit: number } | { $project: Document } | { $addFields: Document }; interface AggregateParams extends PaginationParams { aggregation?: AggregationStage[]; // ... rest of the interface }src/search.ts (1)
6-11: Consider strengthening thenextparameter type.The
Encodabletype fornextmight be too permissive. Consider creating a more specific type that matches the expected structure[number, any].export interface SearchParams { query?: Record<string, any>; limit?: number; fields?: Record<string, number>; next?: [number, any] | string; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
package.json(4 hunks)src/aggregate.ts(1 hunks)src/find.ts(1 hunks)src/search.ts(1 hunks)src/utils/sanitizeParams.ts(4 hunks)test/utils/sanitizeParams.test.ts(1 hunks)tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/sanitizeParams.ts
🧰 Additional context used
📓 Learnings (1)
src/find.ts (2)
Learnt from: adborroto
PR: mixmaxhq/mongo-cursor-pagination#367
File: src/find.ts:14-20
Timestamp: 2025-01-07T11:51:25.488Z
Learning: The `hint` property is defined in the `PaginationParams` interface and is inherited by `FindParams` through extension, so it should not be redefined in `FindParams`.
Learnt from: adborroto
PR: mixmaxhq/mongo-cursor-pagination#367
File: src/find.ts:14-20
Timestamp: 2025-01-07T11:51:25.488Z
Learning: The `hint` property is defined in `PaginationParams` as an optional string property (`hint?: string`). Any interface that extends `PaginationParams`, such as `FindParams` or `AggregateParams`, automatically inherits this property through TypeScript's interface extension.
🔇 Additional comments (3)
test/utils/sanitizeParams.test.ts (1)
1-13: LGTM! Well-structured test setup with proper type definitions.The test file setup is clean with proper TypeScript imports and mock collection initialization.
src/aggregate.ts (1)
46-53: Previous issue with undefined options is fixed correctly.The spread operator now safely handles undefined options with a fallback to an empty object.
src/search.ts (1)
71-80:⚠️ Potential issueAdd type assertion for
params.nextarray access.The array access on
params.nextneeds type safety to prevent runtime errors.if (params.next) { aggregatePipeline.push({ $match: { $or: [ - { score: { $lt: params.next[0] } }, - { score: { $eq: params.next[0] }, _id: { $lt: params.next[1] } }, + { score: { $lt: (params.next as [number, any])[0] } }, + { score: { $eq: (params.next as [number, any])[0] }, _id: { $lt: (params.next as [number, any])[1] } }, ], }, });Likely invalid or redundant comment.
Signed-off-by: Alejandro Dominguez <adborroto90@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
package.json (1)
Line range hint
76-78: Update Node.js version constraint.The current engines field specifies Node.js ">= 6.9.1" which is incompatible with:
- TypeScript 4.9.5 (requires Node.js >= 12.20)
- Modern development tools and dependencies
Update the engines field to reflect current requirements:
"engines": { - "node": ">= 6.9.1" + "node": ">= 14.17.0" }
♻️ Duplicate comments (1)
package.json (1)
54-59: 🛠️ Refactor suggestionAddress TypeScript dependency configuration issues.
Previous review comments have identified several issues that need attention:
- Move
@types/expressto devDependencies- Pin TypeScript-related package versions for consistency
- Ensure compatibility with current Node.js releases
Additionally:
- Add
tslibas a runtime dependency to optimize the TypeScript helpers:"dependencies": { "base64-url": "^2.2.0", + "tslib": "^2.6.2", // ... }Also applies to: 71-73
🧹 Nitpick comments (4)
package.json (1)
14-20: LGTM! Consider adding type declaration file generation.The TypeScript-related script changes look good, with proper separation of concerns and comprehensive testing setup.
Consider adding the
--declarationflag to generate.d.tsfiles in the build script:- "build": "rm -rf dist/ && tsc" + "build": "rm -rf dist/ && tsc --declaration"src/find.ts (3)
22-25: Consider using string literal types for better type safety.Convert the constants to TypeScript string literal types to get better type checking and autocompletion.
-const COLLECTION_METHODS = { +const COLLECTION_METHODS = { FIND: 'find', FIND_AS_CURSOR: 'findAsCursor', -}; +} as const; +type CollectionMethod = typeof COLLECTION_METHODS[keyof typeof COLLECTION_METHODS];
31-32: Add null check for params.fields access.The condition could throw if
params.fieldsis undefined. Consider adding a null check.const removePaginatedFieldInResponse = - params.fields && !params.fields[params.paginatedField || '_id']; + params.fields != null && !params.fields[params.paginatedField || '_id'];
74-75: Consider combining cursor operations.The cursor operations could be chained for better readability.
- const cursor = collatedQuery.sort($sort).limit(params.limit + 1); - if (params.hint) cursor.hint(params.hint); + const cursor = collatedQuery + .sort($sort) + .limit(params.limit + 1) + .hint(params.hint || {});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
package.json(3 hunks)src/find.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/find.ts (2)
Learnt from: adborroto
PR: mixmaxhq/mongo-cursor-pagination#367
File: src/find.ts:14-20
Timestamp: 2025-01-07T11:51:25.488Z
Learning: The `hint` property is defined in the `PaginationParams` interface and is inherited by `FindParams` through extension, so it should not be redefined in `FindParams`.
Learnt from: adborroto
PR: mixmaxhq/mongo-cursor-pagination#367
File: src/find.ts:14-20
Timestamp: 2025-01-07T11:51:25.488Z
Learning: The `hint` property is defined in `PaginationParams` as an optional string property (`hint?: string`). Any interface that extends `PaginationParams`, such as `FindParams` or `AggregateParams`, automatically inherits this property through TypeScript's interface extension.
🪛 Biome (1.9.4)
src/find.ts
[error] 91-91: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
🔇 Additional comments (2)
src/find.ts (2)
14-20: Well-structured interface definition!The
FindParamsinterface properly extendsPaginationParamsand defines clear, type-safe optional properties. Good job on not redefining thehintproperty since it's inherited fromPaginationParams.
44-45: Type assertion could be unsafe.The type assertion to
SanitizeParamsmight hide type incompatibilities. Consider adding runtime validation or using a type guard.✅ Verification successful
Type assertion is safe in this context
The type assertion
params as SanitizeParamsis safe because:
- The
sanitizeParams()function performs thorough runtime validation and transformation- All properties in
SanitizeParamsare optionalFindParamsis compatible withSanitizeParams(shares common properties)- The function returns a properly validated object
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the definition of SanitizeParams to ensure compatibility ast-grep --pattern 'interface SanitizeParams { $$$ }'Length of output: 654
Script:
#!/bin/bash # Check the params parameter type and sanitizeParams implementation ast-grep --pattern 'function find($$$) { $$$ }' echo "=== Checking sanitizeParams implementation ===" ast-grep --pattern 'export async function sanitizeParams($$$) { $$$ }'Length of output: 229
Script:
#!/bin/bash # Check find.ts content echo "=== Content of find.ts ===" cat src/find.ts echo -e "\n=== Content of sanitizeParams.ts ===" cat src/utils/sanitizeParams.tsLength of output: 7355
| function hasFindAsCursor( | ||
| collection: unknown | ||
| ): collection is Collection<Document> & { findAsCursor: Function } { | ||
| return typeof (collection as any).findAsCursor === 'function'; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety of the type guard function.
The function uses unsafe type practices:
- Uses broad 'Function' type (flagged by static analysis)
- Uses type assertion to 'any'
function hasFindAsCursor(
collection: unknown
-): collection is Collection<Document> & { findAsCursor: Function } {
- return typeof (collection as any).findAsCursor === 'function';
+): collection is Collection<Document> & { findAsCursor: (...args: any[]) => any } {
+ return typeof (collection as Partial<Collection<Document>>).findAsCursor === 'function';
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function hasFindAsCursor( | |
| collection: unknown | |
| ): collection is Collection<Document> & { findAsCursor: Function } { | |
| return typeof (collection as any).findAsCursor === 'function'; | |
| } | |
| function hasFindAsCursor( | |
| collection: unknown | |
| ): collection is Collection<Document> & { findAsCursor: (...args: any[]) => any } { | |
| return typeof (collection as Partial<Collection<Document>>).findAsCursor === 'function'; | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 91-91: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/find.ts (1)
118-122:⚠️ Potential issueImprove type safety of the type guard function.
The
Functiontype is too broad and flagged by static analysis. Use a more specific function signature.function hasFindAsCursor( collection: unknown -): collection is Collection<Document> & { findAsCursor: Function } { - return typeof (collection as any).findAsCursor === 'function'; +): collection is Collection<Document> & { findAsCursor: (...args: any[]) => any } { + return typeof (collection as Partial<Collection<Document>>).findAsCursor === 'function'; }🧰 Tools
🪛 Biome (1.9.4)
[error] 120-120: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
🧹 Nitpick comments (5)
src/search.ts (3)
6-11: Consider adding JSDoc comments for the interface properties.The
SearchParamsinterface would benefit from property-level documentation to explain the purpose and constraints of each field.export interface SearchParams { + /** Additional query filters to apply alongside the text search */ query?: Record<string, any>; + /** Number of results per page (1 to config.MAX_LIMIT) */ limit?: number; + /** Fields to include in the results */ fields?: Record<string, number>; + /** Cursor for pagination, encoded using bsonUrlEncoding */ next?: Encodable; }
38-40: Add type assertion for decoded cursor.The decoded cursor should be explicitly typed as
[number, any]to match its usage in the match stage.if (params.next) { - params.next = bsonUrlEncoding.decode(params.next as string); + params.next = bsonUrlEncoding.decode(params.next as string) as [number, any]; }
47-48: Use nullish coalescing for limit checks.The current implementation might not handle
0correctly. Consider using nullish coalescing.- if (params.limit! < 1) params.limit = 1; - if (params.limit! > config.MAX_LIMIT) params.limit = config.MAX_LIMIT; + params.limit = Math.max(1, Math.min(params.limit ?? config.MAX_LIMIT, config.MAX_LIMIT));src/aggregate.ts (2)
12-17: Consider using more specific types for aggregation stages.The
aggregationproperty usesDocument[]which is too generic. Consider creating a union type of possible aggregation stages.type AggregationStage = | { $match: Document } | { $sort: Document } | { $limit: number } | { $project: Document } | { $addFields: Document }; interface AggregateParams extends PaginationParams { aggregation?: AggregationStage[]; // ... rest of the properties }
73-80: Consider extracting case-insensitive pipeline stages.The case-insensitive pipeline construction could be moved to a separate function for better readability.
function buildCaseInsensitivePipeline(params: AggregateParams, $match: Document, $sort: Document, $limit: number): Document[] { return params.aggregation.concat([ { $addFields: { __lc: { $toLower: `$${params.paginatedField}` } } }, { $match }, { $sort }, { $limit }, { $project: { __lc: 0 } }, ]); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/aggregate.ts(2 hunks)src/find.ts(3 hunks)src/search.ts(1 hunks)
🧰 Additional context used
📓 Learnings (2)
src/search.ts (1)
Learnt from: adborroto
PR: mixmaxhq/mongo-cursor-pagination#367
File: src/search.ts:0-0
Timestamp: 2025-01-07T13:38:36.860Z
Learning: In the mongo-cursor-pagination project, the `next` parameter in search and pagination interfaces should use the `Encodable` type from `bsonUrlEncoding` to ensure proper typing of encoded/decoded values.
src/find.ts (2)
Learnt from: adborroto
PR: mixmaxhq/mongo-cursor-pagination#367
File: src/find.ts:14-20
Timestamp: 2025-01-07T11:51:25.488Z
Learning: The `hint` property is defined in the `PaginationParams` interface and is inherited by `FindParams` through extension, so it should not be redefined in `FindParams`.
Learnt from: adborroto
PR: mixmaxhq/mongo-cursor-pagination#367
File: src/find.ts:14-20
Timestamp: 2025-01-07T11:51:25.488Z
Learning: The `hint` property is defined in `PaginationParams` as an optional string property (`hint?: string`). Any interface that extends `PaginationParams`, such as `FindParams` or `AggregateParams`, automatically inherits this property through TypeScript's interface extension.
🪛 Biome (1.9.4)
src/find.ts
[error] 120-120: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
🔇 Additional comments (1)
src/find.ts (1)
43-49: 🛠️ Refactor suggestionRemove redundant
limitproperty.The
limitproperty is already defined inPaginationParams. Remove it to avoid property shadowing.export interface FindParams extends PaginationParams { query?: Document; - limit?: number; fields?: Record<string, number>; collation?: Record<string, any> | null; overrideFields?: Record<string, number>; }Likely invalid or redundant comment.
| const aggregateMethod = 'aggregateAsCursor' in collection ? 'aggregateAsCursor' : 'aggregate'; | ||
| const results = await collection[aggregateMethod](aggregatePipeline).toArray(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use type-safe method detection.
Similar to the hasFindAsCursor function in find.ts, consider adding a type guard for aggregateAsCursor.
function hasAggregateAsCursor(
collection: unknown
): collection is Collection<Document> & { aggregateAsCursor: (...args: any[]) => AggregationCursor<Document> } {
return typeof (collection as Partial<Collection<Document>>).aggregateAsCursor === 'function';
}
const aggregateMethod = hasAggregateAsCursor(collection) ? 'aggregateAsCursor' : 'aggregate';
adborroto
force-pushed
the
adborroto/PLAT-5
branch
from
5c9dbb3 to
e98640e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/find.ts (1)
118-122: 🛠️ Refactor suggestionImprove type safety of the type guard function.
The function uses unsafe type practices:
- Uses broad 'Function' type (flagged by static analysis)
- Uses type assertion to 'any'
🧰 Tools
🪛 Biome (1.9.4)
[error] 120-120: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
🧹 Nitpick comments (9)
src/search.ts (3)
19-32: Enhance TypeScript documentation.Consider adding TypeScript-specific details to the JSDoc:
- Add
@template Tto document the generic type parameter- Add return type information using
@returns {Promise<SearchResponse<T>>}
38-40: Improve type safety when decoding the next parameter.The type assertion to string could be avoided by using type narrowing.
- params.next = bsonUrlEncoding.decode(params.next as string); + if (typeof params.next === 'string') { + params.next = bsonUrlEncoding.decode(params.next); + }
95-100: Improve null safety in response handling.Consider adding explicit type checks for
scoreand_idproperties.if (fullPageOfResults) { const lastResult = _.last(results); - if (lastResult) { + if (lastResult?.score !== undefined && lastResult?._id !== undefined) { response.next = bsonUrlEncoding.encode([lastResult.score, lastResult._id]); } }src/find.ts (1)
74-74: Improve type safety in parameter sanitization.Consider creating a proper type guard for the sanitization process instead of using type assertion.
- params = _.defaults(await sanitizeParams(collection, params as SanitizeParams), { query: {} }); + params = _.defaults(await sanitizeParams(collection, params), { query: {} });Update the
sanitizeParamsfunction signature to acceptFindParams:async function sanitizeParams(collection: Collection<Document>, params: FindParams | SanitizeParams): Promise<SanitizeParams>src/aggregate.ts (5)
57-64: Improve type safety in parameter handling.The type cast to
SanitizeParamssuggests a potential type mismatch. Consider these improvements:
- Define a proper type relationship between
AggregateParamsandSanitizeParams- Replace
_.defaultswith a type-safe alternative- params = _.defaults(await sanitizeParams(collection, params as SanitizeParams), { + params = { + aggregation: [], + ...await sanitizeParams(collection, params as SanitizeParams) + };
68-68: Make the limit calculation more explicit.The limit calculation could be clearer about its purpose of fetching an extra document for pagination.
- const $limit = (params.limit || 0) + 1; + const $limit = (params.limit ?? 0) + 1; // Add 1 to determine if there's a next page
70-81: Consider extracting pipeline segments for better maintainability.The conditional pipeline construction could be more maintainable by extracting the common and variant parts.
const basePipeline = [{ $match }, { $sort }, { $limit }]; const caseInsensitivePipeline = [ { $addFields: { __lc: { $toLower: `$${params.paginatedField}` } } }, ...basePipeline, { $project: { __lc: 0 } } ]; aggregationPipeline = params.aggregation.concat( params.sortCaseInsensitive ? caseInsensitivePipeline : basePipeline );
86-91: Simplify collation handling logic.The collation handling logic can be more concise while maintaining the same behavior.
- const isCollationNull = params.collation === null; - const collation = params.collation || config.COLLATION; - if (collation && !isCollationNull) { - options.collation = collation; - } + if (params.collation !== null) { + options.collation = params.collation || config.COLLATION; + }
93-99: Improve type safety of aggregation method selection.The dynamic method selection could benefit from better type safety and error handling.
type AggregateCollection = Collection<Document> & { aggregateAsCursor?: (...args: any[]) => AggregationCursor<Document>; }; function getAggregateMethod(collection: AggregateCollection) { return 'aggregateAsCursor' in collection ? 'aggregateAsCursor' : 'aggregate'; } const aggregateMethod = getAggregateMethod(collection);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/aggregate.ts(2 hunks)src/find.ts(3 hunks)src/search.ts(1 hunks)
🧰 Additional context used
📓 Learnings (2)
src/find.ts (2)
Learnt from: adborroto
PR: mixmaxhq/mongo-cursor-pagination#367
File: src/find.ts:14-20
Timestamp: 2025-01-07T11:51:25.488Z
Learning: The `hint` property is defined in the `PaginationParams` interface and is inherited by `FindParams` through extension, so it should not be redefined in `FindParams`.
Learnt from: adborroto
PR: mixmaxhq/mongo-cursor-pagination#367
File: src/find.ts:14-20
Timestamp: 2025-01-07T11:51:25.488Z
Learning: The `hint` property is defined in `PaginationParams` as an optional string property (`hint?: string`). Any interface that extends `PaginationParams`, such as `FindParams` or `AggregateParams`, automatically inherits this property through TypeScript's interface extension.
src/search.ts (1)
Learnt from: adborroto
PR: mixmaxhq/mongo-cursor-pagination#367
File: src/search.ts:0-0
Timestamp: 2025-01-07T13:38:36.860Z
Learning: In the mongo-cursor-pagination project, the `next` parameter in search and pagination interfaces should use the `Encodable` type from `bsonUrlEncoding` to ensure proper typing of encoded/decoded values.
🪛 Biome (1.9.4)
src/find.ts
[error] 120-120: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
🔇 Additional comments (4)
src/search.ts (2)
6-17: Well-structured TypeScript interfaces!The interfaces are properly typed with generics support and the
Encodabletype for thenextparameter aligns with the project's conventions.
87-88: Use type-safe method detection.Similar to the
hasFindAsCursorfunction in find.ts, consider adding a type guard foraggregateAsCursor.src/find.ts (1)
43-49: Well-structured interface definition!The
FindParamsinterface properly extendsPaginationParamsand correctly inherits thehintproperty as per the project's conventions.src/aggregate.ts (1)
1-17: LGTM! Well-structured type definitions and imports.The conversion to TypeScript is clean with proper type imports and a well-defined
AggregateParamsinterface that extendsPaginationParams.
eduardborges
left a comment
eduardborges
left a comment
There was a problem hiding this comment.
I reviewed half of the code. I'll keep reviewing it, but I wanted to send you some feedback.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/aggregate.ts (3)
12-17: Consider using more specific types for better type safety.The interface definition could benefit from more specific types:
aggregationcould use a union type of possible aggregation stagesoptionscould useAggregateOptionsfrom mongodb instead ofRecord<string, any>interface AggregateParams extends PaginationParams { - aggregation?: Document[]; - options?: Record<string, any>; + aggregation?: Array<{ + $match?: Document; + $sort?: Document; + $limit?: number; + $project?: Document; + $addFields?: Document; + [key: string]: any; + }>; + options?: AggregateOptions; before?: string; collation?: Record<string, any> | null; }
62-62: Consider using type validation instead of type assertion.The type assertion
params as SanitizeParamsbypasses TypeScript's type checking. Consider implementing a type guard or validation function to ensure type safety at runtime.- params = _.defaults(await sanitizeParams(collection, params as SanitizeParams), { + params = _.defaults(await sanitizeParams(collection, validateParams(params)), {Add a validation function:
function validateParams(params: AggregateParams): SanitizeParams { // Add runtime validation here if (!params || typeof params !== 'object') { throw new Error('Invalid params'); } return params as SanitizeParams; }
101-106: Improve type safety of aggregation method selection.The dynamic method selection could benefit from better type definitions to ensure type safety.
- const aggregateMethod = 'aggregateAsCursor' in collection ? 'aggregateAsCursor' : 'aggregate'; - const cursor: AggregationCursor<Document> = collection[aggregateMethod]( + type AggregateMethod = 'aggregateAsCursor' | 'aggregate'; + const aggregateMethod: AggregateMethod = 'aggregateAsCursor' in collection ? 'aggregateAsCursor' : 'aggregate'; + const cursor: AggregationCursor<Document> = (collection[aggregateMethod] as Function)(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/aggregate.ts(3 hunks)
🔇 Additional comments (2)
src/aggregate.ts (2)
68-81: LGTM! Pipeline construction is well-structured.The conditional pipeline construction handles case-insensitive sorting appropriately, and the limit calculation is properly safeguarded.
Line range hint
87-98: LGTM! Options and collation handling is robust.The implementation properly handles undefined options and null collation cases, with clear documentation about collation usage.
src/utils/query.ts
Outdated
| * @param params - Pagination parameters | ||
| * @param response - The response object to modify | ||
| */ | ||
| export function encodePaginationTokens<T>( |
There was a problem hiding this comment.
I see we changed the parameters that this function receives. Is this a breaking change?
There was a problem hiding this comment.
Great catch 😨 , used in other places
https://github.com/search?q=user%3Amixmaxhq+encodePaginationTokens&type=code&ref=advsearch
Solved on 6559852
| import { MongoMemoryServer } from 'mongodb-memory-server'; | ||
| import { Db, MongoClient } from 'mongodb'; | ||
| import mongoist from 'mongoist'; | ||
| import { MongoClient as MongoClientV3 } from 'mongodbv3'; |
There was a problem hiding this comment.
It seems this is a new addition. What is this about?
There was a problem hiding this comment.
We were using dynamic import that is not supported by ts, this was just importing manually instead .
test/mongoosePlugin.test.ts
Outdated
| it('returns data in the expected format', async () => { | ||
| const data = await Post.paginate(); | ||
| const data = await (Post as any).paginate(); | ||
| var hasOwnProperty = Object.prototype.hasOwnProperty; |
test/mongoosePlugin.test.ts
Outdated
| it('returns data in the expected format for search function', async () => { | ||
| const data = await Post.search('Post #1', { limit: 3 }); | ||
| const data = await (Post as any).search('Post #1', { limit: 3 }); | ||
| var hasOwnProperty = Object.prototype.hasOwnProperty; |
There was a problem hiding this comment.
Should this be a const? How did this work before if this variable was not defined?
There was a problem hiding this comment.
I have the same question 😂 . Apparently a built-in function.
| @@ -1760,7 +1761,6 @@ describe('find', () => { | |||
| paginatedField, | |||
| }); | |||
|
|
|||
| expect(res.results.start).toEqual(undefined); // Verify it is not returned since it is not requested. | |||
There was a problem hiding this comment.
Why did you delete all these expect statements?
There was a problem hiding this comment.
The test was wrong, results is an array do not have property start but since was not typed undefined was correct making the test past.
| * the results. | ||
| */ | ||
| module.exports = async function(collection, searchString, params) { | ||
| if (_.isString(params.limit)) params.limit = parseInt(params.limit, 10); |
There was a problem hiding this comment.
This line is no longer present. Should we keep it to make we don't break anything with JS code?
There was a problem hiding this comment.
Added again just in case, but had to cast to any then to strign since limit is a number for ts
src/mongoose.plugin.ts
Outdated
| * @param params - Query parameters for the pagination. | ||
| * @returns The paginated results. | ||
| */ | ||
| const findFn = async function(this: any, params: FindParams): Promise<any> { |
There was a problem hiding this comment.
This doesn't need to be async unless you do an await
src/mongoose.plugin.ts
Outdated
| * @returns The search results. | ||
| */ | ||
| const searchFn = async function( | ||
| this: any, |
There was a problem hiding this comment.
What's this this? Also, this doesn't need to be async
| } else { | ||
| // Need to repeat `params.paginatedField` default value ('_id') since it's set in 'sanitizeParams()' | ||
| params = _.defaults(await sanitizeParams(collection, params), { query: {} }); | ||
| params = _.defaults(await sanitizeParams(collection, params as SanitizeParams), { query: {} }); |
There was a problem hiding this comment.
Why do we force the use of thet ype SanitizeParams here? Should we just update the type in the parameters of the function?
There was a problem hiding this comment.
SanitizeParams is reused for aggregate and search, which have different params types.
| if (params.sortCaseInsensitive) { | ||
| // For case-insensitive sorting, we need to work with an aggregation: | ||
| response = aggregate( | ||
| response = await aggregate( |
There was a problem hiding this comment.
Not really because since was returning the response , was OK but if we add something else before returning will become a bug, this will reduce the possibility that someone if the future makes a mistake.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
src/search.ts (3)
19-37: Enhance documentation with performance considerations.While the documentation is comprehensive, consider adding specific performance implications:
- Mention that
$textindex scans can be resource-intensive- Note that sorting by
textScorerequires examining all matching documents/** * Performs a search query on a Mongo collection and pages the results. This is different from * find() in that the results are ordered by their relevancy, and as such, it does not take * a paginatedField parameter. Note that this is less performant than find() because it must * perform the full search on each call to this function. + * + * Performance considerations: + * - Requires a full collection scan when using $text index + * - Sorting by textScore examines all matching documents + * - Consider using compound indexes for better performance with additional query filters * @param collection - A MongoDB collection object. This MUST have a Mongo $text index.
38-50: Improve type safety in parameter handling.The current implementation uses type assertions and non-null assertions that could be made more type-safe.
- if (_.isString(params.limit)) params.limit = parseInt((params.limit as any) as string, 10); + if (_.isString(params.limit)) params.limit = parseInt(params.limit, 10); params = _.defaults(params, { query: {}, limit: config.MAX_LIMIT, }); - if (params.limit! < 1) params.limit = 1; - if (params.limit! > config.MAX_LIMIT) params.limit = config.MAX_LIMIT; + const limit = Math.max(1, Math.min(params.limit, config.MAX_LIMIT)); + params.limit = limit;
96-101: Add defensive checks for score and _id access.When encoding the next cursor, ensure score and _id exist on the last result to prevent potential runtime errors.
if (fullPageOfResults) { const lastResult = _.last(results); - if (lastResult) { + if (lastResult?.score !== undefined && lastResult?._id !== undefined) { response.next = bsonUrlEncoding.encode([lastResult.score, lastResult._id]); } }test/utils/query.test.ts (3)
1-2: Consider adding type imports for better type safety.Since this is now a TypeScript file, consider importing the types for the parameters and response objects to ensure type safety in the tests.
import bsonUrlEncoding from '../../src/utils/bsonUrlEncoding'; import { encodePaginationTokens, generateCursorQuery } from '../../src/utils/query'; +import type { PaginationParams, PaginationResponse } from '../../src/utils/types';
10-14: Add assertions for new pagination properties.The test includes new properties
hasPrevious,hasNext, andresults, but doesn't verify their values after encoding. Consider adding assertions to ensure these properties remain unchanged after encoding.encodePaginationTokens(params, response); expect(response.next).toEqual(bsonUrlEncoding.encode('789')); expect(response.previous).toEqual(bsonUrlEncoding.encode('456')); + expect(response.hasNext).toBe(false); + expect(response.hasPrevious).toBe(false); + expect(response.results).toEqual([]);
Line range hint
1-60: Enhance TypeScript adoption in test file.While the file has been converted to TypeScript, it's not taking full advantage of TypeScript features. Consider:
- Adding type annotations for test parameters and variables
- Using typed test fixtures
- Implementing proper typing for mock objects
Example implementation:
interface TestParams extends PaginationParams { paginatedField: string; } interface TestResponse extends PaginationResponse { results: unknown[]; previous: Record<string, unknown>; next: Record<string, unknown>; } describe('encodePaginationTokens', () => { it('encodes the pagination tokens on the passed-in response object', () => { const params: TestParams = { paginatedField: '_id', }; const response: TestResponse = { // ... rest of the test }; }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/mongoose.plugin.ts(1 hunks)src/search.ts(1 hunks)src/utils/query.ts(1 hunks)test/mongoosePlugin.test.ts(4 hunks)test/utils/query.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/mongoose.plugin.ts
- src/utils/query.ts
🧰 Additional context used
📓 Learnings (1)
src/search.ts (1)
Learnt from: adborroto
PR: mixmaxhq/mongo-cursor-pagination#367
File: src/search.ts:0-0
Timestamp: 2025-01-07T13:38:36.860Z
Learning: In the mongo-cursor-pagination project, the `next` parameter in search and pagination interfaces should use the `Encodable` type from `bsonUrlEncoding` to ensure proper typing of encoded/decoded values.
🪛 Biome (1.9.4)
test/mongoosePlugin.test.ts
[error] 70-70: Do not shadow the global "hasOwnProperty" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
[error] 91-91: Do not shadow the global "hasOwnProperty" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (6)
test/mongoosePlugin.test.ts (3)
1-3: LGTM! Clean conversion to ES module imports.The migration from CommonJS
requireto ES moduleimportstatements is well-executed and follows TypeScript best practices.
17-17: LGTM! Correct mongoose ObjectId type usage.The update to use
mongoose.Schema.Types.ObjectIdis the proper way to type ObjectId references in mongoose schemas.
59-64: Avoid usingas anytype assertions.Multiple instances of
as anytype assertions reduce TypeScript's type safety benefits. This is typically a sign that proper types need to be defined.Also applies to: 69-70, 80-81, 85-86, 90-91
src/search.ts (3)
1-17: Well-structured type definitions!The interfaces are properly typed with generics and the
nextparameter correctly uses theEncodabletype as recommended.
51-82: LGTM! Aggregation pipeline is well-structured.The pipeline correctly implements text search with proper sorting and pagination logic.
88-89: Use type-safe method detection.Similar to the previous review suggestion, consider adding a type guard for
aggregateAsCursor.
| it('returns data in the expected format', async () => { | ||
| const data = await Post.paginate(); | ||
| const data = await (Post as any).paginate(); | ||
| const hasOwnProperty = Object.prototype.hasOwnProperty; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Modernize property checks using Object.hasOwn.
Instead of shadowing the global hasOwnProperty and using call, we can use the modern Object.hasOwn() method which is both safer and more concise.
Apply this improvement:
- const hasOwnProperty = Object.prototype.hasOwnProperty;
- expect(hasOwnProperty.call(data, 'results')).toBe(true);
+ expect(Object.hasOwn(data, 'results')).toBe(true);This change should be applied to both instances (lines 70 and 91).
Also applies to: 91-91
🧰 Tools
🪛 Biome (1.9.4)
[error] 70-70: Do not shadow the global "hasOwnProperty" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
| it('generates a cursor query for a paginated field that is not _id', () => { | ||
| const params = { | ||
| paginatedField: 'name', | ||
| next: '123', | ||
| }; | ||
| const query = generateCursorQuery(params); | ||
| expect(query).toEqual({ | ||
| $or: [{ name: { $lt: '1' } }, { name: null }, { _id: { $lt: '2' }, name: { $eq: '1' } }], | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review and fix the cursor query test case.
The test case for cursor query generation has several issues:
- The expected query structure doesn't match typical MongoDB pagination patterns
- The test data (
'123') appears to be raw rather than properly encoded - Missing test cases for important scenarios
Consider these improvements:
- Fix the expected query structure:
expect(query).toEqual({
- $or: [{ name: { $lt: '1' } }, { name: null }, { _id: { $lt: '2' }, name: { $eq: '1' } }],
+ $or: [
+ { name: { $gt: decodedName } },
+ { name: { $eq: decodedName }, _id: { $gt: decodedId } }
+ ]
});- Add proper cursor encoding:
- next: '123',
+ next: bsonUrlEncoding.encode(['TestName', 'TestId']),- Add test cases for:
- Previous cursor
- Both next and previous cursors
- Edge cases with null values
- Different comparison operators based on sort order
Would you like me to provide the additional test cases?
Committable suggestion skipped: line range outside the PR's diff.
ahmed-anas
left a comment
ahmed-anas
left a comment
There was a problem hiding this comment.
Good job! Some comments mini comments are there, please review before proceeding
| response = prepareResponse(results, params); | ||
| } | ||
|
|
||
| // Remove fields that we added to the query (such as paginatedField and _id) that the user didn't ask for. |
There was a problem hiding this comment.
Comment is inconsistent with the subsequent implementation - we only remove paginatedField. do we also want to remove _id? or perhaps the comment is wrong and should be updated.
though I realize this is not part of the js to ts conversion. and within the scope of js-to-ts conversion the code behavior is consistent
| previousPaginatedField = previousPaginatedField?.toLowerCase?.() ?? ''; | ||
| } | ||
| if (shouldSecondarySortOnId) { | ||
| if ( |
There was a problem hiding this comment.
Maybe simplify a bit?
| if ( | |
| if (response.previous?._id) |
There was a problem hiding this comment.
Not possible since previous have many types some that do not include _id property
| nextPaginatedField = nextPaginatedField?.toLowerCase?.() ?? ''; | ||
| } | ||
| if (shouldSecondarySortOnId) { | ||
| if (typeof response.next === 'object' && response.next !== null && '_id' in response.next) { |
There was a problem hiding this comment.
Can we simplify?
| if (typeof response.next === 'object' && response.next !== null && '_id' in response.next) { | |
| if (response.next?._id) { |
There was a problem hiding this comment.
Can't do that, since next have many types, string included , he is not able to parse property unless line 67
| if (shouldSecondarySortOnId) { | ||
| if (typeof response.next === 'object' && response.next !== null && '_id' in response.next) { | ||
| response.next = bsonUrlEncoding.encode([nextPaginatedField, response.next._id]); | ||
| } |
There was a problem hiding this comment.
similar to previous comment, we now have a case where we don't override/populate response.next. is this intentional? are you sure it's ok?
There was a problem hiding this comment.
We can only do it if we have _id, since TS we have to go type safe here.
| @@ -1760,7 +1761,6 @@ describe('find', () => { | |||
| paginatedField, | |||
| }); | |||
|
|
|||
| expect(res.results.start).toEqual(undefined); // Verify it is not returned since it is not requested. | |||
adborroto
force-pushed
the
adborroto/PLAT-5
branch
from
6559852 to
0040cd1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
test/utils/query.test.ts (1)
Line range hint
1-39: LGTM! Consider adding edge cases.The test cases look good with proper TypeScript types. Consider adding test cases for:
- Empty results array
- Null/undefined values in paginated fields
- Case-insensitive sorting scenarios
src/search.ts (1)
51-82: Add type safety to aggregation pipeline.Consider using MongoDB types for better type safety:
- const aggregatePipeline: Document[] = [ + const aggregatePipeline: mongodb.PipelineStage[] = [src/utils/query.ts (3)
4-16: Strengthen type definitions.Consider these type improvements:
- Use literal types for sort direction
- Add constraints to limit values
export type PaginationParams = { paginatedField?: string; sortCaseInsensitive?: boolean; - sortAscending?: boolean; + sortAscending?: true | false; - limit?: number; + limit?: number & { _brand: 'ValidLimit' };
50-54: Simplify null checks using optional chaining.The nested conditions can be simplified:
- if ( - typeof response.previous === 'object' && - response.previous !== null && - '_id' in response.previous - ) { + if (response.previous?._id) {
141-142: Improve type safety and documentation in cursor query generation.Consider these improvements:
- Add proper TypeScript parameter and return types
- Document the behavior for each special case
-function generateCursorQuery(params: any): any { +function generateCursorQuery(params: PaginationParams): Record<string, unknown> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/mongoose.plugin.ts(1 hunks)src/search.ts(1 hunks)src/utils/query.ts(1 hunks)test/mongoosePlugin.test.ts(4 hunks)test/utils/query.test.ts(2 hunks)
🧰 Additional context used
📓 Learnings (1)
src/search.ts (1)
Learnt from: adborroto
PR: mixmaxhq/mongo-cursor-pagination#367
File: src/search.ts:0-0
Timestamp: 2025-01-07T13:38:36.860Z
Learning: In the mongo-cursor-pagination project, the `next` parameter in search and pagination interfaces should use the `Encodable` type from `bsonUrlEncoding` to ensure proper typing of encoded/decoded values.
🪛 Biome (1.9.4)
test/mongoosePlugin.test.ts
[error] 70-70: Do not shadow the global "hasOwnProperty" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
[error] 91-91: Do not shadow the global "hasOwnProperty" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (13)
test/mongoosePlugin.test.ts (4)
1-3: LGTM! Clean migration to ES module syntax.The conversion from CommonJS
requireto ES moduleimportstatements follows TypeScript best practices.
17-17: LGTM! Improved type precision.The update to
mongoose.Schema.Types.ObjectIdprovides better type safety and follows mongoose's recommended type references.
59-59: Avoid usingas anytype assertions.Multiple instances of
as anytype assertions reduce TypeScript's type safety benefits. This is typically a sign that proper types need to be defined.Consider creating proper type definitions for the mongoose plugin:
declare module 'mongoose' { interface Model<T> { paginate(): Promise<PaginationResult<T>>; paginateFN(): Promise<PaginationResult<T>>; search(query: string, options?: SearchOptions): Promise<SearchResult<T>>; searchFN(query: string, options?: SearchOptions): Promise<SearchResult<T>>; } }Also applies to: 64-64, 69-69, 80-80, 85-85, 90-90
70-70: Modernize property checks using Object.hasOwn.Instead of shadowing the global
hasOwnPropertyand usingcall, we can use the modernObject.hasOwn()method which is both safer and more concise.Apply this improvement:
- const hasOwnProperty = Object.prototype.hasOwnProperty; - expect(hasOwnProperty.call(data, 'results')).toBe(true); + expect(Object.hasOwn(data, 'results')).toBe(true);Also applies to: 91-91
🧰 Tools
🪛 Biome (1.9.4)
[error] 70-70: Do not shadow the global "hasOwnProperty" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
src/mongoose.plugin.ts (6)
1-9: Clean TypeScript definitions and imports!The interface definition and import statements follow TypeScript best practices.
16-16: Add generic type parameter to plugin functionThe plugin function should be generic to properly type the model context.
45-55: Clean implementation of plugin registration!The logic for registering static methods based on options is well-implemented and type-safe.
35-43: 🛠️ Refactor suggestionApply the same type safety improvements to searchFn
The search function has the same issues as findFn regarding typing and unnecessary Promise wrapping.
- const searchFn = function(this: any, searchString: string, params: SearchParams): Promise<any> { + const searchFn = function<T>(this: Model<T>, searchString: string, params: SearchParams): PaginationResponse<T> { if (!this.collection) { throw new Error('collection property not found'); } params = _.extend({}, params); - return search(this.collection, searchString, params); + return search(this.collection, searchString, params) as PaginationResponse<T>; };Likely invalid or redundant comment.
21-28: 🛠️ Refactor suggestionImprove type safety and remove unnecessary Promise wrapping
- The
thiscontext and return type should be properly typed- The function doesn't use
await, so it shouldn't return a Promise directly- const findFn = function(this: any, params: FindParams): Promise<any> { + const findFn = function<T>(this: Model<T>, params: FindParams): PaginationResponse<T> { if (!this.collection) { throw new Error('collection property not found'); } params = _.extend({}, params); - return find(this.collection, params); + return find(this.collection, params) as PaginationResponse<T>; };Likely invalid or redundant comment.
3-4: Verify imported types exist in their modulesLet's ensure the imported types are properly defined in their respective modules.
✅ Verification successful
Types are properly defined and exported
BothFindParamsandSearchParamsare correctly defined as interfaces in their respective modules and properly exported.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if FindParams and SearchParams types are defined ast-grep --pattern 'export (type|interface) FindParams' ./src/find.ts ast-grep --pattern 'export (type|interface) SearchParams' ./src/search.tsLength of output: 147
Script:
#!/bin/bash # First check if the files exist fd "^(find|search)\.ts$" src/ # Then use a more flexible pattern to find type definitions echo "=== Searching for FindParams ===" rg "FindParams" src/find.ts -A 2 || true echo "=== Searching for SearchParams ===" rg "SearchParams" src/search.ts -A 2 || trueLength of output: 715
test/utils/query.test.ts (1)
41-59: Review and fix the cursor query test case.The test case for cursor query generation has several issues:
- The expected query structure doesn't match typical MongoDB pagination patterns
- The test data (
'123') appears to be raw rather than properly encoded- Missing test cases for important scenarios
Consider these improvements:
expect(query).toEqual({ - $or: [{ name: { $lt: '1' } }, { name: null }, { _id: { $lt: '2' }, name: { $eq: '1' } }], + $or: [ + { name: { $gt: decodedName } }, + { name: { $eq: decodedName }, _id: { $gt: decodedId } } + ] });src/search.ts (2)
1-17: LGTM! Well-structured TypeScript interfaces.The interfaces are properly typed and correctly use the
Encodabletype for thenextparameter as per previous learnings.
88-89: Use type-safe method detection.Similar to the
hasFindAsCursorfunction in find.ts, consider adding a type guard foraggregateAsCursor.
| if (_.isString(params.limit)) params.limit = parseInt((params.limit as any) as string, 10); | ||
| if (params.next) { | ||
| params.next = bsonUrlEncoding.decode(params.next as string); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety in parameter parsing.
The current implementation has potential type safety issues:
- The string to number conversion could result in NaN
- The type assertion
as any as stringis not type-safe
Consider this safer implementation:
- if (_.isString(params.limit)) params.limit = parseInt((params.limit as any) as string, 10);
+ if (_.isString(params.limit)) {
+ const parsed = parseInt(params.limit, 10);
+ params.limit = Number.isNaN(parsed) ? config.DEFAULT_LIMIT : parsed;
+ }Committable suggestion skipped: line range outside the PR's diff.
📚 Context/Description Behind The Change
This pull request converts the entire project from JavaScript to TypeScript. The conversion includes adding type annotations, interfaces, and updating the build process to compile TypeScript files.
Motivation and Context:
The conversion to TypeScript enhances type safety, improves developer tooling, and helps prevent runtime errors by catching issues at compile time. This change is part of our ongoing effort to modernize the codebase and improve maintainability.
Types of Changes:
🚨 Potential Risks & What To Monitor After Deployment
🧑🔬 How Has This Been Tested?
🚚 Release Plan
Merge and run
npm run semantic-releaseover master.