-
Notifications
You must be signed in to change notification settings - Fork 22
fix(telemetry): support TypeScript exactOptionalPropertyTypes compiler option #270
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces a new Nix development environment configuration file (.idx/dev.nix) for workspace setup, updates the CHANGELOG with a bug fix entry for TypeScript exactOptionalPropertyTypes support, and modifies the TelemetryConfig interface to explicitly allow undefined on the metrics property. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes The changes are straightforward: a new configuration file with guided scaffolding, a documentation entry, and a minimal type definition update. No complex logic, behavioral changes, or multi-file refactoring patterns present. Possibly related issues
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
telemetry/configuration.ts (2)
25-25: Consider adding| undefinedfor consistency.The
attributesproperty inTelemetryMetricConfigis also optional. For completeexactOptionalPropertyTypessupport and consistency with theTelemetryConfig.metricsupdate, consider adding| undefinedhere as well.Apply this diff:
- attributes?: Set<TelemetryAttribute>; + attributes?: Set<TelemetryAttribute> | undefined;
110-111: Update constructor parameter signature for consistency.The constructor parameter
metricsshould also include| undefinedto match the interface property type on line 35. This ensures full consistency and proper support for theexactOptionalPropertyTypescompiler option.Apply this diff:
constructor( - public metrics?: Partial<Record<TelemetryMetric, TelemetryMetricConfig>>, + public metrics?: Partial<Record<TelemetryMetric, TelemetryMetricConfig>> | undefined, ) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.idx/dev.nix(1 hunks)CHANGELOG.md(1 hunks)telemetry/configuration.ts(1 hunks)
🔇 Additional comments (3)
CHANGELOG.md (1)
6-9: LGTM! Documentation correctly reflects the bug fix.The changelog entry accurately documents the TypeScript compiler option compatibility fix.
.idx/dev.nix (1)
1-55: Clarify the inclusion of this unrelated development environment file.This Nix configuration file appears unrelated to the PR's stated objective of supporting TypeScript's
exactOptionalPropertyTypescompiler option. Consider moving this to a separate PR focused on development environment setup.telemetry/configuration.ts (1)
35-35: LGTM! Correctly supports exactOptionalPropertyTypes.The addition of
| undefinedto themetricsproperty ensures compatibility with TypeScript's--exactOptionalPropertyTypescompiler option.
9f75c31 to
1693be9
Compare
|
Hi team I’ve verified that this change builds successfully and passes all telemetry-related tests (npm run build and npm run test telemetry). This update ensures that the SDK compiles cleanly when using the --exactOptionalPropertyTypes TypeScript compiler option. Thank you for reviewing — I appreciate your time and feedback! |
e79f653 to
1693be9
Compare
Summary
This PR makes the telemetry module compatible with the TypeScript compiler option exactOptionalPropertyTypes.
When --exactOptionalPropertyTypes is enabled, TypeScript requires optional properties to explicitly allow undefined. Some telemetry-related type definitions did not conform, causing compilation errors for downstream users.
Changes
Updated telemetry configuration types to include | undefined where appropriate.
Verified that the SDK builds successfully with --exactOptionalPropertyTypes.
Confirmed that all telemetry tests pass (npm run test telemetry).
Why
This change improves SDK compatibility with strict TypeScript projects and ensures the SDK can compile cleanly with modern compiler settings.
Validation
✅ Ran npm run build successfully
✅ Verified telemetry test suite passes
✅ Confirmed no behavioral regressions
Changelog
fix(telemetry): support TypeScript exactOptionalPropertyTypes compiler option
Summary by CodeRabbit
Bug Fixes
Chores