Fix error handling and Buffer parsing in store implementations #1439
Conversation
…tations Several JavaScript changes were needed to satisfy the type checker: - Cast catch block errors to any (e.g., catch (/** @type {any} */ error)) to access error.code and error.statusCode properties - Call Buffer.toString() before JSON.parse in fileAttachmentStore - Rename unused parameters with underscore prefix (_result, _coordinates) to signal intentional non-use - Wrap forEach callbacks in braces to avoid returning delete expression - Use Type suffix for typedef imports to avoid shadowing require() consts The abstract store base classes now accept both EntityCoordinates and ResultCoordinates in list/get methods, matching how subclasses actually use them.
JamieMagee
force-pushed
the
ts-migration-stores
branch
from
b8411ed to
3e045ac
Compare
There was a problem hiding this comment.
Pull request overview
This PR improves type safety and error handling across the store implementations by adding comprehensive TypeScript definitions and JSDoc annotations. The changes address several type-checker issues and fix a potential bug with Buffer handling.
Changes:
- Added TypeScript definition files (.d.ts) for 12 store implementation files to improve type checking
- Fixed Buffer.toString() calls before JSON.parse() in attachment and harvest stores to prevent potential runtime errors
- Improved error handling by properly typing caught errors with
@type {any}annotations where error properties are accessed - Replaced eslint-disable comments with underscore-prefixed parameter names for unused parameters
- Added comprehensive JSDoc documentation to all store classes and methods
- Used
Typesuffix for typedef imports to avoid shadowing require() constants
Reviewed changes
Copilot reviewed 13 out of 30 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | Added new .d.ts and .js files to TypeScript configuration |
| trimmedMongoDefinitionStore.js/.d.ts | Added TypeScript definitions and JSDoc, wrapped forEach callback, renamed unused parameters |
| mongoConfig.js/.d.ts | Added TypeScript definitions and JSDoc for factory functions |
| mongo.js/.d.ts | Added TypeScript definitions, JSDoc, wrapped forEach callback, used bracket notation for property access |
| fileHarvestStore.js/.d.ts | Added TypeScript definitions, JSDoc, fixed Buffer.toString(), typed error catches |
| fileDefinitionStore.js/.d.ts | Added TypeScript definitions, JSDoc, typed error catches, renamed unused parameters |
| fileConfig.js/.d.ts | Added TypeScript definitions and JSDoc for factory functions |
| fileAttachmentStore.js/.d.ts | Added TypeScript definitions, JSDoc, fixed Buffer.toString(), typed error catches |
| dispatchDefinitionStore.js/.d.ts | Added TypeScript definitions and JSDoc for dispatch store |
| dispatchConfig.js/.d.ts | Added TypeScript definitions and JSDoc for dispatch configuration |
| azblobHarvestStore.js/.d.ts | Added TypeScript definitions, JSDoc, renamed unused parameter |
| azblobDefinitionStore.js/.d.ts | Added TypeScript definitions, JSDoc, typed error catches |
| azblobConfig.js/.d.ts | Added TypeScript definitions and JSDoc for Azure Blob configuration |
| azblobAttachmentStore.js/.d.ts | Added TypeScript definitions, JSDoc, typed error catches |
| abstractMongoDefinitionStore.d.ts | Changed export pattern from export default to export = |
| abstractFileStore.d.ts | Changed export pattern and updated type signatures |
| abstractAzblobStore.d.ts | Changed export pattern from export default to export = |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const definitionDoc = clone(definition) | ||
| definitionDoc._id = this.getId(definition.coordinates) | ||
| delete definitionDoc.files | ||
| delete definitionDoc['files'] |
There was a problem hiding this comment.
Inconsistent property access notation: This line uses bracket notation definitionDoc['files'], but similar delete operations elsewhere (e.g., line 61 with def._id and line 80 in mongo.js with def._mongo) use dot notation. Consider using dot notation for consistency unless there's a specific reason to use bracket notation.
| delete definitionDoc['files'] | |
| delete definitionDoc.files |
| await cursor.forEach( | ||
| /** @param {any} page */ page => { | ||
| if (!definition) definition = page | ||
| else definition.files = definition.files.concat(page['files']) |
There was a problem hiding this comment.
Inconsistent property access notation: This line uses bracket notation page['files'], but similar property access elsewhere in the file (e.g., line 80 with def._mongo) uses dot notation. Consider using dot notation for consistency unless there's a specific reason to use bracket notation.
| else definition.files = definition.files.concat(page['files']) | |
| else definition.files = definition.files.concat(page.files) |
| const list = await super.list( | ||
| coordinates, | ||
| /** @param {BlobEntry} entry */ entry => { | ||
| const path = entry.metadata['id'] |
There was a problem hiding this comment.
Inconsistent property access notation: This line uses bracket notation entry.metadata['id'], but dot notation is more commonly used elsewhere in the codebase. Consider using entry.metadata.id for consistency unless there's a specific reason to use bracket notation.
| const path = entry.metadata['id'] | |
| const path = entry.metadata.id |
anyso we can checkerror.codeanderror.statusCodeBuffer.toString()beforeJSON.parse()in fileAttachmentStore (was passing Buffer directly)_result,_coordinates) instead of eslint-disable commentsforEachcallback in braces to avoid returning the result ofdeleteexpressionTypesuffix for typedef imports to avoid shadowingrequire()constantsWhy these changes
The existing code worked fine at runtime but had patterns that confused the type checker. For example, catching an error and immediately accessing
error.codefails because caught values areunknownby default. The Buffer issue was a real bug waiting to happen on some Node versions.