Convert providers/curation/ to TypeScript#1549
Open
JamieMagee wants to merge 4 commits intots-providers/harvestfrom
Open
Convert providers/curation/ to TypeScript#1549JamieMagee wants to merge 4 commits intots-providers/harvestfrom
JamieMagee wants to merge 4 commits intots-providers/harvestfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Converts the providers/curation/ modules from paired .js + .d.ts files into consolidated .ts sources, updating runtime and test imports accordingly.
Changes:
- Replaced
providers/curation/process.js+process.d.tswithprocess.ts(typed queue processor interfaces included). - Converted curation store/config modules (
memoryStore,mongoCurationStore,mongoConfig,githubConfig,azureQueueConfig) to TypeScript and removed their.d.tscompanions. - Updated application/tests/type imports to reference the new
.tsmodules.
Show a summary per file
| File | Description |
|---|---|
| test/providers/curation/processTest.ts | Updates test import to process.ts. |
| test/providers/curation/mongoCurationStore.ts | Updates test import to mongoCurationStore.ts. |
| test/providers/curation/githubPRs.ts | Updates test imports and esmock target to github.ts. |
| test/providers/curation/github.ts | Updates test imports to github.ts/memoryStore.ts. |
| providers/index.js | Switches curation provider imports from .js to .ts. |
| providers/curation/process.ts | New TS implementation replacing prior .js + .d.ts. |
| providers/curation/process.js | Removed legacy JS implementation. |
| providers/curation/process.d.ts | Removed legacy declaration file (now inlined via TS). |
| providers/curation/mongoCurationStore.ts | Type migration and minor error-handling changes. |
| providers/curation/mongoCurationStore.d.ts | Removed legacy declaration file. |
| providers/curation/mongoConfig.ts | Updated imports/types for TS store module. |
| providers/curation/mongoConfig.d.ts | Removed legacy declaration file. |
| providers/curation/memoryStore.ts | Type migration for in-memory store. |
| providers/curation/memoryStore.d.ts | Removed legacy declaration file. |
| providers/curation/githubConfig.ts | Type migration and updated imports for TS GitHub service. |
| providers/curation/githubConfig.d.ts | Removed legacy declaration file. |
| providers/curation/github.ts | Type migration for GitHub curation service. |
| providers/curation/github.d.ts | Removed legacy declaration file. |
| providers/curation/azureQueueConfig.ts | Type migration and updated imports for TS queue module. |
| providers/curation/azureQueueConfig.d.ts | Removed legacy declaration file. |
| bin/config.d.ts | Updates type-only import to process.ts. |
| app.js | Updates runtime import to process.ts. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
providers/curation/memoryStore.ts:35
getContributionreturnsundefinedwhen a PR isn’t present, but the curation store contract (ICurationStore.getContribution) documents returningnullwhen not found. Returningnullhere would keep behavior consistent across store implementations and simplify callers.
providers/curation/mongoCurationStore.ts:55- The error handling assumes the thrown value is an
Error((error as Error).message). If a non-Error is thrown, this can logundefinedor even throw while handling the exception. Consider usingerror instanceof Error ? error.message : String(error)(and optionally include the full error as metadata) before callingretry.
- Files reviewed: 14/22 changed files
- Comments generated: 1
569f162 to
d912865
Compare
1d826cf to
1b2c208
Compare
17f3527 to
aec304a
Compare
1b2c208 to
9619521
Compare
9619521 to
2751711
Compare
There was a problem hiding this comment.
Pull request overview
Converts the providers/curation/ implementation from paired .js + .d.ts files into unified .ts modules, updating imports across the app and tests accordingly.
Changes:
- Replaced curation provider implementations (
process,github,mongoCurationStore, configs, etc.) with TypeScript equivalents. - Updated application wiring and provider registry imports to reference the new
.tsmodules. - Updated curation-related tests to import the new TypeScript modules and adjusted related comments.
Show a summary per file
| File | Description |
|---|---|
| test/providers/curation/processTest.ts | Updates test import to the new process.ts. |
| test/providers/curation/mongoCurationStore.ts | Updates test import to the new mongoCurationStore.ts. |
| test/providers/curation/githubPRs.ts | Updates imports and esmock target to github.ts. |
| test/providers/curation/github.ts | Updates imports to the new curation TypeScript modules. |
| providers/index.js | Switches curation provider imports from .js to .ts. |
| providers/curation/process.ts | New TS implementation merging prior process.js + process.d.ts. |
| providers/curation/process.js | Removes old JS implementation. |
| providers/curation/process.d.ts | Removes old declaration file (now in TS). |
| providers/curation/mongoCurationStore.ts | Converts Mongo curation store to TS and removes JSDoc typedefs. |
| providers/curation/mongoCurationStore.d.ts | Removes old declaration file (now in TS). |
| providers/curation/mongoConfig.ts | Converts config wiring to TS types/imports. |
| providers/curation/mongoConfig.d.ts | Removes old declaration file (now in TS). |
| providers/curation/memoryStore.ts | Converts in-memory store to TS and adds declarations/types. |
| providers/curation/memoryStore.d.ts | Removes old declaration file (now in TS). |
| providers/curation/githubConfig.ts | Converts GitHub curation config factory to TS typings/imports. |
| providers/curation/githubConfig.d.ts | Removes old declaration file (now in TS). |
| providers/curation/github.ts | Converts GitHub curation service to TS, adding explicit types and some @ts-expect-error bridges. |
| providers/curation/github.d.ts | Removes old declaration file (now in TS). |
| providers/curation/azureQueueConfig.ts | Converts Azure queue config factory to TS typings/imports. |
| providers/curation/azureQueueConfig.d.ts | Removes old declaration file (now in TS). |
| bin/config.d.ts | Updates type-only import to process.ts. |
| app.js | Updates curation processing import to process.ts. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (5)
providers/curation/github.ts:440
autoCurateassumesthis.list()returns aCurationListResult(it is forced via@ts-expect-error), butICurationStore.list()can also returnCurationData[](MemoryStore). If the service is configured with the memory store, this will throw at runtime when accessingcurationAndContributions.curations. Consider normalizinglist()to always returnCurationListResult(e.g., wrapCurationData[]into{ curations, contributions: [] }) or branching onArray.isArray(data)here instead of suppressing the type error.
providers/curation/github.ts:296_calculateMatchingRevisionAndReasonforcesawait this.list(...)into aCurationListResultvia@ts-expect-error, butlist()can returnCurationData[] | nulldepending on the store. This can lead to runtime failures when accessing.curations/.contributionsif the memory store is used. Prefer handling the union result explicitly (e.g.,Array.isArray(...)/ null check) or adjustlist()to consistently returnCurationListResult.
This issue also appears in the following locations of the same file:
- line 438
- line 943
providers/curation/github.ts:954
listAllis declared to returnRecord<string, CurationListResult>, but it assigns the result ofthis.list()which is typed asCurationListResult | CurationData[] | nulland is currently silenced with@ts-expect-error. Either widen the return type (e.g., includeCurationData[]) or normalizethis.list()results so callers always receive aCurationListResult; this will avoid suppressions and make the API contract accurate.
providers/curation/memoryStore.ts:35MemoryStore.getContributionreturnsundefinedfor missing entries, while the curation store contract elsewhere (e.g., Mongo store /ICurationStoredocs) usesnullto represent “not found”. Returningnullhere would make behavior consistent across store implementations and simplify consumers that differentiate between “no doc” vs other falsy values.
providers/curation/mongoCurationStore.ts:55- In the Mongo initialization retry log,
(error as Error).messageassumes the caught value is anError. If a non-Error is thrown (string/object), this logsundefinedand loses useful context. Consider usingerror instanceof Error ? error.message : String(error)(and optionally include the full error object in metadata) before callingretry(error).
- Files reviewed: 14/22 changed files
- Comments generated: 0 new
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Merge 7
.js+.d.tspairs into.tsinproviders/curation/.Review commit-by-commit. Stacked on #1548.