chore: upgrade ts target (perf) #26392
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
…tibility With ES2022 target, class properties with definite assignment assertion (!) are treated as runtime class fields that overwrite base class properties, causing TS2612 errors. Adding the 'declare' modifier makes these properties type-only, preventing runtime field emission while preserving type narrowing. Co-Authored-By: Volnei Munhoz <volnei.munhoz@gmail.com>
…rget Fixed additional TS2612 errors in: - event-type.output.ts: hideCalendarEventDetails property - team.output.ts: parentId property Co-Authored-By: Volnei Munhoz <volnei.munhoz@gmail.com>
With ES2022 target + isolatedModules + emitDecoratorMetadata, TypeScript requires type-only imports for types used in decorated signatures. Co-Authored-By: Volnei Munhoz <volnei.munhoz@gmail.com>
devin-ai-integration
bot
changed the title
…ompatibility When target is ES2022, TypeScript defaults module to ES6 which outputs ESM syntax. This breaks Jest tests that expect CommonJS. Explicitly setting module: CommonJS maintains the previous behavior while still using ES2022 target for syntax features. Co-Authored-By: Volnei Munhoz <volnei.munhoz@gmail.com>
E2E results are ready! |
When target is ES2022, TypeScript defaults module to ES6 which outputs ESM syntax. This breaks Jest tests that expect CommonJS. Explicitly setting module: CommonJS in enums, utils, and types packages maintains the previous behavior while still using ES2022 target for syntax features. Co-Authored-By: Volnei Munhoz <volnei.munhoz@gmail.com>
…S2022 compatibility In ES2022, class fields are initialized in declaration order before the constructor runs. Properties that depend on constructor parameters must be initialized in the constructor, not as field initializers. Co-Authored-By: Volnei Munhoz <volnei.munhoz@gmail.com>
…S2022 compatibility Co-Authored-By: Volnei Munhoz <volnei.munhoz@gmail.com>
…S2022 compatibility in API v2 - Fix TS2729 errors by moving field initializations to constructor body - Fix TS2612 error by adding declare modifier to workflow-step.input.ts - Fix TS2322 error by changing return type in teams.controller.ts - Enable unsafeParameterDecoratorsEnabled in biome.json for NestJS decorators Co-Authored-By: Volnei Munhoz <volnei.munhoz@gmail.com>
…22 compatibility Co-Authored-By: Volnei Munhoz <volnei.munhoz@gmail.com>
…S2022 class fields
With ES2022 target, class fields are emitted as runtime properties even when undefined.
This caused Object.keys() to include properties like maximumActiveBookings and offerReschedule
even when the user only sent { disabled: false }, making the validation incorrectly pass.
The fix checks for properties with defined values instead of just checking for key existence.
Co-Authored-By: Volnei Munhoz <volnei.munhoz@gmail.com>
The ES2022 target works without explicit module: CommonJS setting. Also removed unnecessary comment from validator fix. Co-Authored-By: Volnei Munhoz <volnei.munhoz@gmail.com>
* feat: add type=module to platform packages for ESM migration Co-Authored-By: Volnei Munhoz <volnei.munhoz@gmail.com> * refactor: remove CommonJS from platform-libraries and convert to ESM imports Co-Authored-By: Volnei Munhoz <volnei.munhoz@gmail.com> --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Changes SummaryThis PR upgrades the TypeScript compilation target to ES2022 across the monorepo to reduce transpilation overhead and improve performance. The changes include updating tsconfig files, adjusting class field initialization patterns to align with ES2022 semantics, and migrating platform packages to ESM (type=module). Type: refactoring Components Affected: API v2 services (Calendar, Conferencing, Teams, Stripe, Workflows), TypeScript configuration (base, Next.js, React Library), Platform packages (constants, enums, types, libraries, utils), Embeds (core, react, snippet), TRPC server, Biome linter configuration, Validation utilities Files Changed
Architecture Impact
Risk Areas: Runtime behavior changes: ES2022 class field semantics differ from earlier targets - class fields are now initialized before constructor, requiring deferred initialization for config-dependent values, NestJS DTO decorators: Using declare modifier on decorated properties requires verification that decorators still function correctly at runtime, ESM migration: Platform packages switching to type=module may cause import resolution issues in consuming code, Validation logic: RequiresAtLeastOnePropertyWhenNotDisabled validator fix depends on correct handling of undefined fields in ES2022, Module resolution: i18n.ts using createRequire in ESM context requires careful testing of i18n functionality Suggestions
Full review in progress... | Powered by diffray |
| const translationCache = new Map<string, Record<string, string>>([["en-common", englishTranslations]]); | ||
| const i18nInstanceCache = new Map<string, I18nInstance>(); |
There was a problem hiding this comment.
🟠 HIGH - Module-level mutable caches without cleanup strategy
Agent: nodejs
Category: bug
Description:
Two module-level Maps (translationCache, i18nInstanceCache) accumulate data indefinitely. translationCache.set() called at line 55, i18nInstanceCache.set() at line 90. No size limits, no TTL, no cleanup mechanism.
Suggestion:
Implement cache management with LRU eviction, TTL-based expiration, or document maximum size limits. Use lru-cache package or add periodic cleanup mechanism.
Confidence: 85%
Rule: node_module_level_state
Review ID: 0d8706e4-cb4f-47ad-8896-885907bfafa3
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| // Button added here triggers the modal on click. So, it has to have the same data attributes as the modal target as well | ||
| dataset!: DOMStringMap & FloatingButtonDataset & ModalTargetDatasetProps; | ||
| declare dataset: DOMStringMap & FloatingButtonDataset & ModalTargetDatasetProps; | ||
| buttonWrapperStyleDisplay!: HTMLElement["style"]["display"]; |
There was a problem hiding this comment.
🟠 HIGH - Uninitialized property access on toggle callback
Agent: bugs
Category: bug
Description:
Property buttonWrapperStyleDisplay declared with ! assertion at line 46 but never initialized. When data-toggle-off callback triggers with off=false before first toggle-off=true event, this.buttonWrapperStyleDisplay is undefined at line 84.
Suggestion:
Initialize buttonWrapperStyleDisplay in constructor or provide fallback: buttonWrapperEl.style.display = off ? 'none' : (this.buttonWrapperStyleDisplay ?? 'block');
Confidence: 85%
Rule: ts_validate_nullable_before_use
Review ID: 0d8706e4-cb4f-47ad-8896-885907bfafa3
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| "main": "./dist/index.ts", | ||
| "types": "./dist/index.ts", |
There was a problem hiding this comment.
🟠 HIGH - Incorrect main and types entry points with TS source files
Agent: Delegated (nodejs, performance, quality, react)
Category: bug
Description:
package.json specifies main and types pointing to './dist/index.ts' (TypeScript source file) instead of compiled output. However, exports field at lines 8-13 correctly points to .js and .d.ts.map files, which may override these.
Suggestion:
Update package.json entries: main: './dist/index.js', types: './dist/index.d.ts' for consistency with exports field.
Confidence: 75%
Review ID: 0d8706e4-cb4f-47ad-8896-885907bfafa3
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| "version": "0.0.0", | ||
| "type": "module", | ||
| "main": "./dist/index.js", | ||
| "types": "./dist/index.js", |
There was a problem hiding this comment.
🟠 HIGH - Incorrect types entry point pointing to compiled JS instead of .d.ts
Agent: Delegated (nodejs, performance, quality, react)
Category: bug
Description:
The types entry points to './dist/index.js' (compiled JavaScript) instead of './dist/index.d.ts' (TypeScript declarations). TypeScript uses the 'types' field to resolve type information.
Suggestion:
Update package.json: types: './dist/index.d.ts' to correctly point to TypeScript declaration files.
Confidence: 90%
Review ID: 0d8706e4-cb4f-47ad-8896-885907bfafa3
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| { | ||
| "extends": "@calcom/tsconfig/base.json", | ||
| "compilerOptions": { | ||
| "target": "ES5", | ||
| "target": "ES2022", | ||
| "resolveJsonModule": true, | ||
| "experimentalDecorators": true, | ||
| "baseUrl": "./", |
There was a problem hiding this comment.
🟠 HIGH - Missing TypeScript project reference configuration
Agent: react
Category: quality
Description:
tsconfig.json is missing composite, declaration, and declarationMap settings. Package depends on @calcom/platform-constants (line 26 in package.json) but lacks proper project reference configuration for monorepo builds.
Suggestion:
Add to compilerOptions: "composite": true, "declaration": true, "declarationMap": true, and add 'references' array.
Confidence: 70%
Rule: ts_enable_ts_project_references_for_depende
Review ID: 0d8706e4-cb4f-47ad-8896-885907bfafa3
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| /* eslint-enable @typescript-eslint/no-require-imports */ | ||
|
|
||
| const translationCache = new Map<string, Record<string, string>>([["en-common", englishTranslations]]); | ||
| const i18nInstanceCache = new Map<string, I18nInstance>(); |
There was a problem hiding this comment.
🟡 MEDIUM - Hardcoded locale transformation without configuration
Agent: quality
Category: quality
Description:
Locale transformation for 'zh' to 'zh-CN' is hardcoded. Should be configurable for maintainability.
Suggestion:
Define const LOCALE_TRANSFORMATIONS = { 'zh': 'zh-CN' } and use it.
Why this matters: Simpler code is easier to understand, test, and maintain. Reducing unnecessary abstraction lowers cognitive load and reduces places where bugs can hide. Standard library functions are well-tested and familiar to most developers.
Confidence: 60%
Rule: quality_prefer_simple_readable_code
Review ID: 0d8706e4-cb4f-47ad-8896-885907bfafa3
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| client_secret: z.string(), | ||
| }); | ||
|
|
||
| @Injectable() |
There was a problem hiding this comment.
🟠 HIGH - CommonJS require() import for qs-stringify
Agent: react
Category: quality
Description:
Using TypeScript's import = require() syntax which compiles to CommonJS. While valid, this pattern may cause issues in pure ESM contexts.
Suggestion:
Convert to ES Module import: 'import stringify from "qs-stringify";' if the package supports ESM.
Confidence: 75%
Rule: ts_use_es_modules_import_export
Review ID: 0d8706e4-cb4f-47ad-8896-885907bfafa3
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| import { createRequire } from "node:module"; | ||
|
|
||
| import { WEBAPP_URL } from "@calcom/lib/constants"; | ||
| import { fetchWithTimeout } from "@calcom/lib/fetchWithTimeout"; | ||
| import logger from "@calcom/lib/logger"; | ||
|
|
||
| /* eslint-disable @typescript-eslint/no-require-imports */ | ||
| const { i18n } = require("@calcom/config/next-i18next.config"); | ||
| const path = require("node:path"); | ||
| const __filename = fileURLToPath(import.meta.url); | ||
| const __dirname = path.dirname(__filename); | ||
| const require = createRequire(import.meta.url); | ||
| const i18nConfig = require("@calcom/config/next-i18next.config"); | ||
| const { i18n } = i18nConfig; | ||
| const translationsPath = path.resolve(__dirname, "../../../../apps/web/public/static/locales/en/common.json"); | ||
| const englishTranslations: Record<string, string> = require(translationsPath); |
There was a problem hiding this comment.
🟠 HIGH - Using createRequire for CommonJS require() in ESM context
Agent: react
Category: quality
Description:
File uses createRequire to load CommonJS modules (next-i18next.config and translation JSON). While valid in Node.js, this may cause issues if full ESM migration is desired.
Suggestion:
Consider using dynamic imports or ensuring config files support ESM. However, this pattern is sometimes necessary for JSON imports in ESM.
Confidence: 65%
Rule: ts_use_es_modules_import_export
Review ID: 0d8706e4-cb4f-47ad-8896-885907bfafa3
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| /* eslint-enable @typescript-eslint/no-require-imports */ | ||
|
|
||
| const translationCache = new Map<string, Record<string, string>>([["en-common", englishTranslations]]); | ||
| const i18nInstanceCache = new Map<string, I18nInstance>(); |
There was a problem hiding this comment.
🟡 MEDIUM - Potential null dereference without validation
Agent: typescript
Category: quality
Description:
Map.get() returns T | undefined even after has() check. TypeScript doesn't narrow the type after has() because of potential race conditions.
Suggestion:
Use: const cached = translationCache.get(cacheKey); if (cached) { return cached; }
Confidence: 70%
Rule: ts_strict_null_checks
Review ID: 0d8706e4-cb4f-47ad-8896-885907bfafa3
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
Review Summary
Validated 46 issues: 26 kept, 20 filtered Issues Found: 26💬 See 9 individual line comment(s) for details. 📊 14 unique issue type(s) across 26 location(s) 📋 Full issue list (click to expand)🔴 CRITICAL - Null pointer dereference in validateDefaultField (2 occurrences)Agent: bugs Category: bug 📍 View all locations
Rule: 🔴 CRITICAL - Unsafe property access on JSON parsed data (2 occurrences)Agent: bugs Category: bug 📍 View all locations
Rule: 🟠 HIGH - Module-level mutable caches without cleanup strategyAgent: nodejs Category: bug File: Description: Two module-level Maps (translationCache, i18nInstanceCache) accumulate data indefinitely. translationCache.set() called at line 55, i18nInstanceCache.set() at line 90. No size limits, no TTL, no cleanup mechanism. Suggestion: Implement cache management with LRU eviction, TTL-based expiration, or document maximum size limits. Use lru-cache package or add periodic cleanup mechanism. Confidence: 85% Rule: 🟠 HIGH - Uninitialized property access on toggle callbackAgent: bugs Category: bug File: Description: Property buttonWrapperStyleDisplay declared with ! assertion at line 46 but never initialized. When data-toggle-off callback triggers with off=false before first toggle-off=true event, this.buttonWrapperStyleDisplay is undefined at line 84. Suggestion: Initialize buttonWrapperStyleDisplay in constructor or provide fallback: buttonWrapperEl.style.display = off ? 'none' : (this.buttonWrapperStyleDisplay ?? 'block'); Confidence: 85% Rule: 🟠 HIGH - Incorrect main and types entry points with TS source filesAgent: Delegated (nodejs, performance, quality, react) Category: bug File: Description: package.json specifies main and types pointing to './dist/index.ts' (TypeScript source file) instead of compiled output. However, exports field at lines 8-13 correctly points to .js and .d.ts.map files, which may override these. Suggestion: Update package.json entries: main: './dist/index.js', types: './dist/index.d.ts' for consistency with exports field. Confidence: 75% 🟠 HIGH - Incorrect types entry point pointing to compiled JS instead of .d.tsAgent: Delegated (nodejs, performance, quality, react) Category: bug File: Description: The types entry points to './dist/index.js' (compiled JavaScript) instead of './dist/index.d.ts' (TypeScript declarations). TypeScript uses the 'types' field to resolve type information. Suggestion: Update package.json: types: './dist/index.d.ts' to correctly point to TypeScript declaration files. Confidence: 90% 🟠 HIGH - Missing TypeScript project reference configurationAgent: react Category: quality File: Description: tsconfig.json is missing composite, declaration, and declarationMap settings. Package depends on @calcom/platform-constants (line 26 in package.json) but lacks proper project reference configuration for monorepo builds. Suggestion: Add to compilerOptions: "composite": true, "declaration": true, "declarationMap": true, and add 'references' array. Confidence: 70% Rule: 🟠 HIGH - CommonJS require() import for qs-stringify (2 occurrences)Agent: react Category: quality 📍 View all locations
Rule: 🟡 MEDIUM - Magic numbers for environment-dependent timeout (2 occurrences)Agent: quality Category: quality Why this matters: Simpler code is easier to understand, test, and maintain. Reducing unnecessary abstraction lowers cognitive load and reduces places where bugs can hide. Standard library functions are well-tested and familiar to most developers. 📍 View all locations
Rule: 🟡 MEDIUM - Loose equality comparison with string literal (2 occurrences)Agent: refactoring Category: quality 📍 View all locations
Rule: 🟡 MEDIUM - Unsafe any type in z.record pattern (2 occurrences)Agent: typescript Category: quality 📍 View all locations
Rule: 🟡 MEDIUM - Unnecessary JSON stringify/parse roundtripAgent: typescript Category: quality File: Description: Converting credentials.key to JSON string and back is unnecessary overhead and can lose type information. Suggestion: Access credentials.key directly with proper typing: const stripeKeyObject = credentials.key as Record<string, unknown> Confidence: 75% Rule: 🟡 MEDIUM - Unsafe type casting chain for metadata access (6 occurrences)Agent: typescript Category: quality 📍 View all locations
Rule: 🟡 MEDIUM - Potential null dereference without validation (2 occurrences)Agent: typescript Category: quality 📍 View all locations
Rule: ℹ️ 17 issue(s) outside PR diff (click to expand)
🔴 CRITICAL - Null pointer dereference in validateDefaultField (2 occurrences)Agent: bugs Category: bug 📍 View all locations
Rule: 🔴 CRITICAL - Unsafe property access on JSON parsed data (2 occurrences)Agent: bugs Category: bug 📍 View all locations
Rule: 🟡 MEDIUM - Magic numbers for environment-dependent timeoutAgent: quality Category: quality Why this matters: Simpler code is easier to understand, test, and maintain. Reducing unnecessary abstraction lowers cognitive load and reduces places where bugs can hide. Standard library functions are well-tested and familiar to most developers. File: Description: Timeout values (30000 vs 3000) are hardcoded without named constants. Makes maintenance harder and unclear why different timeouts are used. Suggestion: Define const DEVELOPMENT_TIMEOUT = 30000; const PRODUCTION_TIMEOUT = 3000; Confidence: 62% Rule: 🟡 MEDIUM - Loose equality comparison with string literal (2 occurrences)Agent: refactoring Category: quality 📍 View all locations
Rule: 🟡 MEDIUM - Unsafe any type in z.record pattern (2 occurrences)Agent: typescript Category: quality 📍 View all locations
Rule: 🟡 MEDIUM - Unnecessary JSON stringify/parse roundtripAgent: typescript Category: quality File: Description: Converting credentials.key to JSON string and back is unnecessary overhead and can lose type information. Suggestion: Access credentials.key directly with proper typing: const stripeKeyObject = credentials.key as Record<string, unknown> Confidence: 75% Rule: 🟡 MEDIUM - Unsafe type casting chain for metadata access (6 occurrences)Agent: typescript Category: quality 📍 View all locations
Rule: 🟡 MEDIUM - Unsafe optional chaining - email may be undefinedAgent: typescript Category: quality File: Description: graphUser.mail ?? graphUser.userPrincipalName assigns undefined to responseBody.email if both properties are undefined. This could cause downstream issues when email is expected. Suggestion: Add validation: if (!graphUser.mail && !graphUser.userPrincipalName) throw new BadRequestException('User email not found') Confidence: 75% Rule: Review ID: |
What does this PR do?
Upgrades the TypeScript target to ES2022 across the monorepo to reduce transpilation and improve performance.
Summary by cubic
Upgraded the TypeScript target to ES2022 across the monorepo to reduce transpilation and improve performance. Moved field initializations into constructors and switched to type-only class properties to align with ES2022 semantics.
Refactors
Bug Fixes
Written for commit d01dac4. Summary will update on new commits.
ES2022 Compatibility Fixes
The ES2022 target changes class field semantics, requiring the following fixes:
TS2612 Errors (Property will overwrite base property):
booking-fields.output.ts- Addeddeclaremodifier to properties in 19 output classesbooking.output.ts- FixedbookingFieldsResponsesinRecurringBookingOutput_2024_08_13event-type.output.ts- FixedhideCalendarEventDetailsinTeamEventTypeOutput_2024_06_14team.output.ts- FixedparentIdinOrgTeamOutputDtoTS1272 Errors (Type imports with decorators):
get-event-types-query.input.ts- Changed toimport typeforSortOrderTypeCI Status
Link to Devin run: https://app.devin.ai/sessions/9bf85eb1d10f4045b6ca88c0ef2c2085
Requested by: Volnei Munhoz (@volnei)
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
yarn type-check:ci --forceto verify no new type errors are introducedyarn lintto verify linting passesyarn testto verify unit tests passHuman Review Checklist
declaremodifier changes don't affect runtime behavior of NestJS DTOs with decorators