EPMRPP-89496 || Migrate client-javascript to TypeScript - #271
EPMRPP-89496 || Migrate client-javascript to TypeScript#271maria-hambardzumian wants to merge 5 commits into
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe pull request migrates the client from CommonJS JavaScript to typed TypeScript, adds public models and constants, updates REST/OAuth/proxy/reporting modules, and changes Jest, compiler, npm packaging, and publishing configuration to build from ChangesTypeScript migration and public contracts
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/oauth.ts (1)
52-60: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHonor the supplied OAuth debug option.
RestClientpassesdebuginto this constructor, but Line 60 ignores it and reads onlyrestClientConfig.debug. Client-level OAuth diagnostics are therefore disabled unless the nested REST option is also set.Proposed fix
- this.debug = this.restClientConfig.debug || false; + this.debug = config.debug ?? this.restClientConfig.debug ?? false;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/oauth.ts` around lines 52 - 60, Update the OAuthInterceptor constructor’s debug assignment to honor the supplied OAuth-level config.debug value passed by RestClient, while preserving the existing restClientConfig.debug fallback when the OAuth option is absent.
🧹 Nitpick comments (2)
jest.config.js (1)
3-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid disabling TypeScript diagnostics without a required CI typecheck.
diagnostics: falselets Jest execute code with broken TypeScript contracts, weakening the migration’s validation. Keep diagnostics enabled or ensure CI runstsc --noEmitas a required check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jest.config.js` at line 3, Update the ts-jest configuration in the TypeScript transform entry to remove diagnostics: false so Jest performs TypeScript diagnostics, or alternatively add a required CI tsc --noEmit check. Preserve the existing tsconfig.json configuration..eslintrc (1)
24-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winScope the disabled import rules instead of disabling them globally.
Turning off
import/no-unresolvedandimport/extensionsfor the entire repository can hide broken imports outside the TypeScript source. Prefer configuring the TypeScript resolver or applying overrides only to the affected files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.eslintrc around lines 24 - 27, Update the ESLint configuration entries for import/no-unresolved and import/extensions to apply only through a TypeScript-specific resolver or targeted file overrides, rather than disabling them repository-wide. Preserve the existing camelcase and valid-jsdoc settings, and ensure unaffected JavaScript files continue receiving import validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@__tests__/oauth.spec.js`:
- Around line 5-7: The axios mock’s isAxiosError implementation in the
jest.mock('axios', ...) block incorrectly identifies errors by response
presence; match real Axios by requiring error.isAxiosError === true, and update
the test’s rejected error value to include isAxiosError: true when it represents
an AxiosError.
In `@src/lib/rest.ts`:
- Around line 53-59: Export the RestClientOptions interface so the
ConstructorParameters<typeof RestClient>[0] type used by helpers.getServerResult
remains publicly nameable during declaration emit. Change only the interface’s
visibility and preserve its existing fields and types.
---
Outside diff comments:
In `@src/lib/oauth.ts`:
- Around line 52-60: Update the OAuthInterceptor constructor’s debug assignment
to honor the supplied OAuth-level config.debug value passed by RestClient, while
preserving the existing restClientConfig.debug fallback when the OAuth option is
absent.
---
Nitpick comments:
In @.eslintrc:
- Around line 24-27: Update the ESLint configuration entries for
import/no-unresolved and import/extensions to apply only through a
TypeScript-specific resolver or targeted file overrides, rather than disabling
them repository-wide. Preserve the existing camelcase and valid-jsdoc settings,
and ensure unaffected JavaScript files continue receiving import validation.
In `@jest.config.js`:
- Line 3: Update the ts-jest configuration in the TypeScript transform entry to
remove diagnostics: false so Jest performs TypeScript diagnostics, or
alternatively add a required CI tsc --noEmit check. Preserve the existing
tsconfig.json configuration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fffa4a88-0e2d-4af0-a88e-987e39f808e7
📒 Files selected for processing (49)
.eslintrc.github/workflows/publish.yml.gitignore__tests__/client-id.spec.js__tests__/config.spec.js__tests__/helpers.spec.js__tests__/oauth.spec.js__tests__/proxyHelper.spec.js__tests__/publicReportingAPI.spec.js__tests__/report-portal-client.spec.js__tests__/rest.spec.js__tests__/statistics.spec.jsindex.d.tsjest.config.jslib/constants/events.jslib/constants/statuses.jslib/publicReportingAPI.jspackage.jsonsrc/constants.tssrc/helpers.tssrc/lib/commons/config.tssrc/lib/commons/errors.tssrc/lib/constants/events.tssrc/lib/constants/index.tssrc/lib/constants/launchModes.tssrc/lib/constants/logLevels.tssrc/lib/constants/outputs.tssrc/lib/constants/statuses.tssrc/lib/constants/testItemTypes.tssrc/lib/helpers.tssrc/lib/logger.tssrc/lib/models/common.tssrc/lib/models/config.tssrc/lib/models/index.tssrc/lib/models/requests.tssrc/lib/models/responses.tssrc/lib/oauth.tssrc/lib/pjson.tssrc/lib/proxyHelper.tssrc/lib/publicReportingAPI.tssrc/lib/report-portal-client.tssrc/lib/rest.tssrc/models.tssrc/statistics/client-id.tssrc/statistics/constants.tssrc/statistics/statistics.tssrc/types/vendor.d.tsstatistics/constants.jstsconfig.json
💤 Files with no reviewable changes (5)
- lib/publicReportingAPI.js
- lib/constants/statuses.js
- statistics/constants.js
- lib/constants/events.js
- index.d.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
__tests__/oauth.spec.js (1)
403-433: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the proxy wiring, not only the log.
This test proves that the proxy branch is entered, but not that
axios.postreceives the appropriate agent andproxy: false. Capture the request config and assert those fields to protect the actual proxy contract.Suggested assertion
expect(consoleSpy).toHaveBeenCalledWith( `[OAuth] Making token request to ${baseConfig.tokenEndpoint} with proxy agent`, '', ); + const requestConfig = axios.post.mock.calls[0][2]; + expect(requestConfig.proxy).toBe(false); + expect(requestConfig.httpsAgent ?? requestConfig.httpAgent).toBeDefined();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/oauth.spec.js` around lines 403 - 433, Update the test “logs the proxied token request when debug and proxy are both enabled” to capture the config passed to axios.post and assert that the request uses the configured proxy agent and sets proxy to false, while preserving the existing token and log assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@__tests__/oauth.spec.js`:
- Around line 403-433: Update the test “logs the proxied token request when
debug and proxy are both enabled” to capture the config passed to axios.post and
assert that the request uses the configured proxy agent and sets proxy to false,
while preserving the existing token and log assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fcf07188-9241-4267-a4c8-e47ac9002ea1
📒 Files selected for processing (7)
.eslintrc__tests__/oauth.spec.js__tests__/rest.spec.jssrc/helpers.tssrc/lib/commons/config.tssrc/lib/models/config.tssrc/lib/rest.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/helpers.ts
- tests/rest.spec.js
- .eslintrc
- src/lib/models/config.ts
- src/lib/commons/config.ts
- src/lib/rest.ts
…96-migrate-to-typescript
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/report-portal-client.ts (1)
375-385: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPropagate REST failures instead of converting them to success.
mergeLaunchescontinues withlaunch/mergeafter a search failure, using an empty launch list, and both merge and file-upload catches resolve normally. This can issue invalid merge requests and make failed log uploads appear successful.Proposed fix
- (error): Array<string | number> => { + (error) => { this.logDebug(`Error during launches search with UUIDs: ${launchUUIds}`, error); console.dir(error); - return []; + throw error; }, ... .catch((error) => { this.logDebug(`Error merging launches with UUIDs: ${launchUUIds}`, error); console.dir(error); + throw error; }); ... .catch((error) => { this.logDebug(`Error save log with file: ${fileObj.name}`, error); console.dir(error); + throw error; });Also applies to: 393-396, 721-734
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/report-portal-client.ts` around lines 375 - 385, Update mergeLaunches and the related file-upload error handlers to rethrow or reject REST failures after logging instead of returning successful fallback values. In the launch UUID search catch, do not continue to getMergeLaunchesRequest or call launch/merge with an empty list; ensure the promise remains rejected. Apply the same failure propagation to the catches around the upload flow at the referenced symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/lib/report-portal-client.ts`:
- Around line 375-385: Update mergeLaunches and the related file-upload error
handlers to rethrow or reject REST failures after logging instead of returning
successful fallback values. In the launch UUID search catch, do not continue to
getMergeLaunchesRequest or call launch/merge with an empty list; ensure the
promise remains rejected. Apply the same failure propagation to the catches
around the upload flow at the referenced symbols.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a827787d-3d6f-46f4-8b06-1698f26f436c
📒 Files selected for processing (3)
__tests__/report-portal-client.spec.jssrc/lib/models/requests.tssrc/lib/report-portal-client.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/report-portal-client.spec.js
- src/lib/models/requests.ts
AmsterGet
left a comment
AmsterGet
left a comment
There was a problem hiding this comment.
Nice work!
Let's polish this before merging.
General comments:
- pay attention to pipeline warnings
- get rid of
any - test this PR with the agent written on plain JS (e.g. Cypress)
| const PJSON_NAME = pjson.name; | ||
|
|
||
| const getUUIDFromFileName = (filename) => filename.match(/rplaunch-(.*)\.tmp/)[1]; | ||
| export interface SystemAttribute { |
There was a problem hiding this comment.
Let's reuse regular Attribute type instead.
| }; | ||
|
|
||
| const getSystemAttribute = () => { | ||
| export const getSystemAttribute = (): SystemAttribute[] => { |
There was a problem hiding this comment.
| export const getSystemAttribute = (): SystemAttribute[] => { | |
| export const getSystemAttributes = (): Attribute[] => { |
| const { response, config } = error; | ||
| const status = response ? response.status : null; | ||
|
|
||
| // eslint-disable-next-line no-console |
There was a problem hiding this comment.
As we are using console's methods as a default logging approach, so maybe it make sense to disable this rule on the config level?
| // eslint-disable-next-line no-console | ||
| console.log(' noProxy from env:', noProxyFromEnv); | ||
| // eslint-disable-next-line no-console | ||
| console.log(' Final noProxy:', noProxy); |
There was a problem hiding this comment.
Let's log it multiline to reduce extra calls.
| /** | ||
| * Public API to emit additional events to RP JS agents. | ||
| */ | ||
| class PublicReportingAPI { |
There was a problem hiding this comment.
Let's create an Interface for this API and export it as the consumer agents often uses their own implementation while maintaining the same interface.
| let rejectFinish; | ||
| const obj = { | ||
| getNewItemObj(startPromiseFunc: PromiseExecutor): ItemObj { | ||
| let resolveFinish!: (value?: any) => void; |
There was a problem hiding this comment.
Can we get rid of any here?
Let's also break the pipeline in case of any used.
| @@ -0,0 +1,3 @@ | |||
| // Public entry point so consumers can import constants as | |||
| // `@reportportal/client-javascript/constants` instead of the internal `/lib/constants` path. | |||
| export * from './lib/constants'; | |||
There was a problem hiding this comment.
I suppose this can be solved using package.json exports feature (example from vitest agent).
| /lib | ||
| /statistics |
There was a problem hiding this comment.
What is the reason for ignoring this stuff?
There was a problem hiding this comment.
These are all build artifacts
There was a problem hiding this comment.
tsc outputs .js/.d.ts/.js.map into the root and /lib (outDir is .). Generated on build, shipped via files
| '!src/lib/constants/index.ts', | ||
| '!src/lib/constants/launchModes.ts', | ||
| '!src/lib/constants/logLevels.ts', | ||
| '!src/lib/constants/testItemTypes.ts', |
There was a problem hiding this comment.
Do we really need to ignore this stuff?
I suppose they should be covered out of the box as they are used from covered files.
There was a problem hiding this comment.
Tried removing them, coverage drops below threshold (branches 76%, functions 73%).
They're type-only models and enum files that aren't exercised at runtime
| ], | ||
| "main": "./lib/report-portal-client", | ||
| "types": "./index.d.ts", | ||
| "types": "./lib/report-portal-client.d.ts", |
There was a problem hiding this comment.
Will this make all types accessible for the package consumers?
There was a problem hiding this comment.
With export = RPClient the root only exposes the client class
The named types come from the subpaths: /models, /constants, /helpers, /publicReportingAPI.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.eslintrc (1)
26-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep unresolved-import validation enabled.
Disabling
import/no-unresolvedglobally can allow brokensrcand package imports to pass lint. Configure the TypeScript resolver instead, or scope this disablement only to files that genuinely require it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.eslintrc at line 26, Remove the global disablement of import/no-unresolved in the ESLint configuration. Configure the TypeScript import resolver so valid TypeScript and package imports resolve correctly, or narrowly scope any remaining disablement only to files that genuinely require it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.eslintrc:
- Line 26: Remove the global disablement of import/no-unresolved in the ESLint
configuration. Configure the TypeScript import resolver so valid TypeScript and
package imports resolve correctly, or narrowly scope any remaining disablement
only to files that genuinely require it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a5b89f07-d93f-49d7-bcdd-c9a2d3ae8559
📒 Files selected for processing (16)
.eslintrc__tests__/helpers.spec.js__tests__/report-portal-client.spec.jspackage.jsonsrc/lib/commons/config.tssrc/lib/constants/outputs.tssrc/lib/helpers.tssrc/lib/logger.tssrc/lib/models/index.tssrc/lib/models/reporting.tssrc/lib/oauth.tssrc/lib/proxyHelper.tssrc/lib/publicReportingAPI.tssrc/lib/report-portal-client.tssrc/lib/rest.tssrc/statistics/statistics.ts
💤 Files with no reviewable changes (6)
- src/lib/logger.ts
- src/lib/constants/outputs.ts
- src/lib/oauth.ts
- src/statistics/statistics.ts
- src/lib/commons/config.ts
- src/lib/rest.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/lib/models/index.ts
- tests/helpers.spec.js
- src/lib/publicReportingAPI.ts
- src/lib/proxyHelper.ts
- src/lib/report-portal-client.ts
2 participants
Summary by CodeRabbit