TypeScript Foundation: Type Definitions and Config (Chunk 1/8 from PR #213) #224
Conversation
This commit establishes the foundation for gradual TypeScript adoption: Changes: - Enhanced global.d.ts with missing Jasmine 1.x type definitions - Added jasmine.Clock.reset() and useMock() - Added jasmine.Spy.andCallFake() - Added global waits() and runs() functions for Jasmine 1.x async tests - Adjusted tsconfig.json to loosen strictness for gradual adoption - Set noImplicitOverride: false - Set noImplicitReturns: false - Set strictFunctionTypes: false - Added emitDeclarationOnly and outDir for d.ts generation - Added standard.globals section to package.json for test globals - Updated type packages to correct versions Impact: - All 955 tests pass ✓ - TypeScript compilation now succeeds with 0 errors ✓ - Sets foundation for incremental source code type annotation Related to PR #213
WalkthroughUpdates to Jasmine TypeScript declarations: adjusted Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🪛 Biome (2.1.2)global.d.ts[error] 30-30: Do not shadow the global "undefined" property. Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global. (lint/suspicious/noShadowRestrictedNames) 🔇 Additional comments (3)
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: 3
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
global.d.tspackage.jsontsconfig.json
🧰 Additional context used
🧬 Code graph analysis (1)
global.d.ts (1)
packages/utils/src/ie.ts (1)
ieVersion(27-27)
🪛 Biome (2.1.2)
global.d.ts
[error] 33-33: Do not shadow the global "undefined" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🪛 GitHub Actions: Check code formatting with Prettier
global.d.ts
[warning] 1-1: Code style issues found by Prettier. Run 'npx prettier --write' to fix.
🪛 GitHub Actions: Check eslint
global.d.ts
[error] 23-23: no-var: Unexpected var, use let or const instead.
🪛 GitHub Check: eslint
global.d.ts
[failure] 78-78:
Prefer using the primitive symbol as a type name, rather than the upper-cased Symbol
[failure] 37-37:
Unexpected var, use let or const instead
[failure] 35-35:
Unexpected var, use let or const instead
[failure] 34-34:
Unexpected var, use let or const instead
[failure] 33-33:
Unexpected var, use let or const instead
[failure] 32-32:
Unexpected var, use let or const instead
[failure] 29-29:
Unexpected var, use let or const instead
[failure] 25-25:
Unexpected var, use let or const instead
[failure] 23-23:
Unexpected var, use let or const instead
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: testheadless (22.x, on)
🔇 Additional comments (6)
tsconfig.json (2)
14-16: Clarify thenoEmitandemitDeclarationOnlyinteraction.Having both
noEmit: trueandemitDeclarationOnly: truetogether means no output will be generated by default—noEmittakes precedence. The comment explains the intent (override viamake dts), but consider whether keepingemitDeclarationOnlyhere serves any purpose whilenoEmitis active, or if it's just pre-configuring for the makefile override.This works as intended if
make dtspasses--noEmit false, but the current config won't emit anything on a plaintscinvocation.
20-25: Relaxed strictness flags align with gradual adoption strategy.Disabling
noImplicitOverride,noImplicitReturns, andstrictFunctionTypesis appropriate for incremental TypeScript migration. This allows the codebase to compile without requiring immediate fixes for all strictness violations.global.d.ts (3)
23-37:vardeclarations are valid in ambient namespace contexts.The ESLint
no-varwarnings are false positives here. In TypeScript ambient declarations (.d.tsfiles),varis the correct keyword for declaring module-scoped variables within anamespace. Usingletorconstwould be semantically incorrect in this context—ambient declarations describe existing JavaScript globals, andvarappropriately represents that they exist without implying block-scoping semantics.Consider adding an ESLint override for this file or suppressing these specific warnings.
66-68: Jasmine 1.x async helpers properly declared.The
waits()andruns()global functions are correctly declared for Jasmine 1.x compatibility, matching the PR objective of supporting legacy async test patterns.
9-16: Window interface augmentation looks good.The Window interface extensions for
DEBUG,amdRequire,require,jQuery, andinnerShivproperly declare globals used in the codebase.package.json (1)
17-31: Test globals configuration aligns with type definitions.The
standard.globalsarray correctly declares the Jasmine/test framework globals (waits,runs,testNode, etc.) that are also typed inglobal.d.ts. This ensures both the linter and TypeScript recognize these identifiers.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
global.d.ts (2)
45-47: Pre-existing issue: DuplicatetoContainHtmldeclarations.Two
toContainHtmlmethod signatures exist with different parameter counts (line 45 with two parameters, line 47 with one). This was flagged in previous reviews. Consolidate into a single signature with an optional second parameter or use proper method overloads.🔎 Suggested fix (from previous review)
- toContainHtml(expectedHtml: any, postProcessCleanedHtml: any): boolean + toContainHtml(expectedHtml: any, postProcessCleanedHtml?: any): boolean toHaveSelectedValues(expectedValues: any): boolean - toContainHtml(expectedValues: any): booleanNote: This issue predates the current PR but remains unresolved.
31-31: Pre-existing issue:undefinedshadows global.This line was flagged in previous reviews and by static analysis. The declaration
const undefined: undefinedshadows the built-in globalundefinedand should be removed or renamed.Note: This issue predates the current PR but remains unresolved.
🧹 Nitpick comments (2)
global.d.ts (2)
60-60: Consider using a more specific function type.The
Functiontype works but is generally discouraged in TypeScript. For better type safety, consider using a more specific signature.🔎 Optional improvement
- andCallFake(fn: Function): Spy + andCallFake(fn: (...args: any[]) => any): SpyThis provides slightly better type information while remaining flexible for various callback signatures.
64-67: LGTM: Jasmine 1.x async helpers correctly defined.The
waits()andruns()functions are properly typed for Jasmine 1.x async test support. The comment correctly notes these are deprecated but still in use.🔎 Optional: More specific function type
Similar to line 60, you could use a more specific function type:
- function runs(fn: Function): void + function runs(fn: () => void): voidHowever, the current typing is acceptable for these legacy helpers.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
global.d.ts
🧰 Additional context used
🧬 Code graph analysis (1)
global.d.ts (1)
packages/utils/src/ie.ts (1)
ieVersion(27-27)
🪛 Biome (2.1.2)
global.d.ts
[error] 31-31: Do not shadow the global "undefined" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: testheadless (22.x, on)
- GitHub Check: testheadless (22.x, off)
🔇 Additional comments (3)
global.d.ts (3)
21-21: LGTM: Jasmine namespace declarations added correctly.These declarations properly type Jasmine 1.x runtime objects. Using
anyforSpecis acceptable when the actual type structure is complex or not relevant to tests.Also applies to: 23-23, 27-27, 35-35
54-55: LGTM: Clock methods match Jasmine 1.x API.The
reset()anduseMock()methods correctly represent the Jasmine 1.x Clock interface.
68-74: LGTM: Documentation provides helpful context.The comments appropriately attribute the Knockout type definitions and explain that most types have been moved to TKO source files.
|
@phillipc I've tagged you to review this for sanity, noting that it's essentially a chunk of #213 that we can review/merge immediately, then we can look at the next chunks (which should all be pretty quick, but 213 wont even really load in Github anymore so this makes the pieces at least reviewable). Thank you! |
tsconfig.json
Outdated
| "noImplicitOverride": true, | ||
| "noImplicitReturns": true, | ||
| "noImplicitOverride": false, | ||
| "noImplicitReturns": false, |
There was a problem hiding this comment.
Since #222 we have made next steps and uses a stricter typescript configuration.
global.d.ts
Outdated
| } | ||
|
|
||
| // Jasmine 1.x async test helpers (deprecated but still used in codebase) | ||
| function waits(timeout?: number): void |
There was a problem hiding this comment.
Oddly enough, the type system has no problems with these fields. In my opinion, this expansions are unnecessary.
|
Thanks those are valuable comments @phillipc . I addressed them and the CodeRabbit comments, so should be good to merge now. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
global.d.tspackage.json
✅ Files skipped from review due to trivial changes (1)
- package.json
🧰 Additional context used
🪛 Biome (2.1.2)
global.d.ts
[error] 30-30: Do not shadow the global "undefined" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: testheadless (22.x, off)
- GitHub Check: testheadless (22.x, on)
🔇 Additional comments (3)
global.d.ts (3)
44-44: LGTM! Clean resolution of duplicate declarations.Making the second parameter optional properly consolidates the two previous
toContainHtmlsignatures into a single, unambiguous declaration.
62-64: LGTM! Well-documented legacy support.The comment clearly identifies these as deprecated Jasmine 1.x async helpers that are still needed in the codebase. This transparent documentation supports the gradual TypeScript adoption strategy outlined in the PR objectives.
52-53: The type definitions forClock.reset(),Clock.useMock(), andSpy.andCallFake()align with Jasmine 1.x API. These methods are documented in Jasmine 1.x and the signatures (void returns for Clock methods, function parameter for Spy.andCallFake) are correct.
global.d.ts
Outdated
| const Spec: any | ||
| const FakeTimer: any | ||
| const undefined: undefined | ||
| const undefined: any // Legacy Jasmine 1.x sentinel value |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for jasmine.undefined usage in test files
rg -n 'jasmine\.undefined' --type=js --type=ts -A 2 -B 2Repository: knockout/tko
Length of output: 1691
🏁 Script executed:
# Search for direct uses of just 'undefined' in test/helper files (not jasmine.undefined)
rg -n '(?<!jasmine\.)\bundefined\b' packages/utils/helpers/ --type=ts --type=js -C 1 | head -50Repository: knockout/tko
Length of output: 287
🏁 Script executed:
# Also check the actual global.d.ts file to see the full context
cat -n global.d.ts | sed -n '20,40p'Repository: knockout/tko
Length of output: 921
🏁 Script executed:
# Search for undefined used without jasmine prefix in the helper files
rg -n '\bundefined\b' packages/utils/helpers/jasmine-13-helper.ts --type=ts -C 1Repository: knockout/tko
Length of output: 401
🏁 Script executed:
# Search more broadly - check if undefined is used in any way that would require the global declaration
rg -n 'undefined' global.d.tsRepository: knockout/tko
Length of output: 120
Consider refactoring to eliminate undefined shadowing.
The global undefined declaration is currently used in packages/utils/helpers/jasmine-13-helper.ts:141 for a nullish check. However, other code in the same file explicitly uses jasmine.undefined (lines 213, 218). To resolve the static analysis flag, refactor the nullish check on line 141 to either use jasmine.undefined consistently or the nullish coalescing operator (!= null) instead of relying on the global undefined declaration.
🧰 Tools
🪛 Biome (2.1.2)
[error] 30-30: Do not shadow the global "undefined" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🤖 Prompt for AI Agents
In global.d.ts line 30 and packages/utils/helpers/jasmine-13-helper.ts around
line 141, eliminate the shadowed global `undefined` by changing the nullish
check at jasmine-13-helper.ts:141 to not rely on the legacy global: either
replace the check with a nullish comparison using != null (e.g., checkValue !=
null) or use the explicit jasmine sentinel by comparing to jasmine.undefined for
consistency with other uses on lines 213 and 218; after updating the check,
remove or stop using the `const undefined: any` declaration in global.d.ts to
resolve the shadowing warning.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all usages of jasmine.undefined in the codebase to understand how it's used
rg -n 'jasmine\.undefined' --type=js --type=ts -C 3
Length of output: 1910
🏁 Script executed:
#!/bin/bash
# Check the specific file mentioned in bot's analysis
cat packages/utils/helpers/jasmine-13-helper.ts | head -230 | tail -30
Length of output: 947
🏁 Script executed:
#!/bin/bash
# Look at the context around line 141 where plain undefined is used
sed -n '135,145p' packages/utils/helpers/jasmine-13-helper.ts
Length of output: 330
@phillipc No, undefined would be more appropriate than any for type safety. Since jasmine.undefined is a sentinel value representing the actual undefined in Jasmine 1.x, typing it as undefined is semantically accurate and gives you proper type checking in the comparisons (line 213) and assignments (line 218).
Using any loses type safety and doesn't provide any benefit here. The Biome warning is about shadowing the global undefined name, not about the type choice itself.
However, as the bot suggested, the best long-term fix would be to eliminate this declaration entirely by refactoring the two usages to either:
- Use standard nullish checks (
!= null) instead of the Jasmine 1.x sentinel - Or keep using
jasmine.undefinedbut accept the type error and cast it where needed
For this PR's scope (type definitions only), I'd recommend:
const undefined: undefined // Legacy Jasmine 1.x sentinel valueThen tackle the refactoring in a future PR if you want to eliminate the Biome warning completely.
🧬 Code Graph Analysis Results
[No analysis requested; providing potentially relevant code snippets for dealing with the user comment.]
File: packages/utils/src/ie.ts, lines 27-27
ieVersion
File: packages/binding.template/src/templateSources.ts, lines 84-97
text(valueToWrite?: string): string | void {
const elemContentsProperty =
this.templateType === templateScript ? 'text' : this.templateType === templateTextArea ? 'value' : 'innerHTML'
if (arguments.length == 0) {
return this.domElement[elemContentsProperty]
} else {
if (elemContentsProperty === 'innerHTML') {
setHtml(this.domElement, valueToWrite!)
} else {
this.domElement[elemContentsProperty] = valueToWrite
}
}
}
File: packages/binding.template/src/templateSources.ts, lines 150-162
override text(/* valueToWrite */): string | undefined {
if (arguments.length == 0) {
const templateData = getTemplateDomData(this.domElement)
if (templateData.textData === undefined && templateData.containerData) {
templateData.textData = templateData.containerData.innerHTML
}
return templateData.textData
} else {
const valueToWrite = arguments[0]
setTemplateDomData(this.domElement, { textData: valueToWrite })
}
return undefined
}
Summary
This is Chunk 1 of 8 breaking down PR #213 into reviewable pieces. This PR establishes the TypeScript foundation by fixing type definitions and adjusting compiler configuration.
Changes
global.d.ts - Enhanced with missing Jasmine 1.x type definitions:
jasmine.Clock.reset()anduseMock()methodsjasmine.Spy.andCallFake()methodwaits()andruns()functions for Jasmine 1.x async teststsconfig.json - Adjusted for gradual TypeScript adoption:
noImplicitOverride: falsenoImplicitReturns: falsestrictFunctionTypes: falseemitDeclarationOnlyandoutDirfor d.ts generationpackage.json - Added standard.globals section declaring test framework globals
Impact
✅ All 955 tests pass
✅ TypeScript compilation succeeds with 0 errors (was 420 on PR #213)
✅ No source code changes - only type definitions and config
✅ Sets foundation for incremental source code type annotations
Breaking Down PR #213
This is part of a strategy to make PR #213 more reviewable:
Related
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.