Configure TypeScript to be stricter #222
Conversation
WalkthroughAdded widespread TypeScript Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/binding.template/src/templateEngine.ts (2)
86-89: Critical: Potential null dereference after error.If
elemis null/undefined (line 86), the code callsoptions.onError()(line 87) but then continues to line 89 where it attempts to constructnew domElement(elem)with a null value. This will cause a runtime error unlessoptions.onError()throws.The error handling is inconsistent with the unknown template type branch (lines 94-95), which explicitly returns
undefinedafter logging the error.🔎 Proposed fix
let elem = templateDocument.getElementById(template) if (!elem) { options.onError(new Error('Cannot find template with ID ' + template)) + return undefined } return new domElement(elem)
81-81: Return type should includeundefinedto match implementation.The function signature declares the return type as
TemplateSource, but line 95 explicitly returnsundefined. This creates a type inconsistency that could lead to runtime errors in calling code that doesn't expect undefined.🔎 Proposed fix
- makeTemplateSource(template: string | Node, templateDocument?: Document) { + makeTemplateSource(template: string | Node, templateDocument?: Document): TemplateSource | undefined {Also applies to: 94-95
packages/binding.core/src/textInput.ts (1)
174-178: Critical: Method name typo detected.Line 174 has
overrideupdateModelwhich appears to be a typo. This should beoverride updateModel(with a space betweenoverrideandupdateModel). The current code likely creates a method with the wrong name or causes a compilation error.🔎 Proposed fix
- overrideupdateModel(...args: [any]) { + override updateModel(...args: [any]) {
🧹 Nitpick comments (3)
packages/utils.parser/src/Parser.ts (3)
102-107: Add type annotations for stricter TypeScript compliance.Since the PR aims to enable stricter TypeScript settings, consider adding explicit type annotations to the
errormethod. The return type should be: neversince this method always throws.🔎 Suggested type annotations
-error(m) { +error(m: string | Error | { name: string; message: string }): never { if (m instanceof Error) { throw m } throw this.createError(m) }
109-113: Document and type the new public method.The new
createErrormethod is a public API addition that lacks documentation and type annotations. Adding these would improve maintainability and align with the stricter TypeScript goals.🔎 Suggested improvements
+/** + * Creates a formatted Error object with position information. + * @param {string | { name: string; message: string }} m Error message or object + * @returns {Error} Formatted Error object + */ -createError(m) { +createError(m: string | { name: string; message: string }): Error { let [name, msg] = m.name ? [m.name, m.message] : [m, ''] const message = `\n${name} ${msg} of ${this.text}\n` + Array(this.at).join(' ') + '_/ 🔥 \\_\n' return new Error(message) }
254-254: Consider unifying the error handling pattern.The codebase now has two patterns for throwing errors:
- Direct throws:
throw this.createError('message')(lines 254, 317, 355, 643)- Indirect throws:
this.error('message')(still used at lines 81, 91, 133, 230, 330, 428, 876)Both are functionally equivalent since
error()always throws. However, for consistency and maintainability, consider:
- Using
this.error()everywhere if you want centralized error handling- Using
throw this.createError()everywhere if you want explicit control flowThe current mixed approach may confuse future maintainers about when to use which pattern.
Also applies to: 317-317, 355-355, 643-643
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (48)
packages/bind/spec/bindingAttributeBehaviors.ts(2 hunks)packages/bind/spec/bindingCompletionPromiseBehavior.ts(2 hunks)packages/bind/spec/bindingDependencyBehaviors.ts(1 hunks)packages/bind/spec/nodePreprocessingBehaviors.ts(5 hunks)packages/bind/src/BindingHandler.ts(1 hunks)packages/bind/src/DescendantBindingHandler.ts(1 hunks)packages/bind/src/LegacyBindingHandler.ts(2 hunks)packages/bind/src/applyBindings.ts(3 hunks)packages/binding.component/spec/componentBindingBehaviors.ts(24 hunks)packages/binding.component/src/componentBinding.ts(1 hunks)packages/binding.component/src/slotBinding.ts(1 hunks)packages/binding.core/spec/optionsBehaviors.ts(1 hunks)packages/binding.core/spec/textBehaviors.ts(2 hunks)packages/binding.core/src/descendantsComplete.ts(1 hunks)packages/binding.core/src/textInput.ts(4 hunks)packages/binding.core/src/value.ts(1 hunks)packages/binding.foreach/spec/eachBehavior.ts(3 hunks)packages/binding.foreach/src/foreach.ts(4 hunks)packages/binding.if/src/ConditionalBindingHandler.ts(1 hunks)packages/binding.if/src/else.ts(1 hunks)packages/binding.if/src/ifUnless.ts(2 hunks)packages/binding.if/src/with.ts(2 hunks)packages/binding.template/spec/foreachBehaviors.ts(3 hunks)packages/binding.template/spec/templatingBehaviors.ts(3 hunks)packages/binding.template/src/foreach.ts(1 hunks)packages/binding.template/src/templateEngine.ts(1 hunks)packages/binding.template/src/templateSources.ts(1 hunks)packages/binding.template/src/templating.ts(1 hunks)packages/computed/src/computed.ts(1 hunks)packages/observable/spec/observableArrayBehaviors.ts(1 hunks)packages/observable/src/dependencyDetection.ts(1 hunks)packages/provider.attr/src/AttributeProvider.ts(2 hunks)packages/provider.bindingstring/spec/BindingStringProviderBehaviors.ts(1 hunks)packages/provider.bindingstring/src/BindingStringProvider.ts(1 hunks)packages/provider.component/src/ComponentProvider.ts(3 hunks)packages/provider.databind/src/DataBindProvider.ts(1 hunks)packages/provider.multi/src/MultiProvider.ts(4 hunks)packages/provider.mustache/spec/attributeInterpolationSpec.ts(1 hunks)packages/provider.mustache/src/AttributeMustacheProvider.ts(3 hunks)packages/provider.mustache/src/TextMustacheProvider.ts(2 hunks)packages/provider.native/src/NativeProvider.ts(3 hunks)packages/provider.virtual/src/VirtualProvider.ts(3 hunks)packages/provider/spec/providerBehaviors.ts(1 hunks)packages/provider/src/Provider.ts(2 hunks)packages/utils.jsx/spec/jsxBehaviors.ts(1 hunks)packages/utils.jsx/src/JsxObserver.ts(1 hunks)packages/utils.parser/src/Parser.ts(5 hunks)tsconfig.json(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-21T09:16:36.575Z
Learnt from: phillipc
Repo: knockout/tko PR: 221
File: packages/bind/spec/bindingAttributeBehaviors.ts:135-138
Timestamp: 2025-12-21T09:16:36.575Z
Learning: In the TKO framework, data-bind expressions may use naked arrow function syntax (zero-parameter arrows written without parentheses), e.g., data-bind='click: => was_clicked(true)' or lambdaLiteral: => null. This is valid TKO syntax, though not standard JavaScript. For code reviews, treat this as an allowed syntax specific to TKO in the analyzed file. Ensure tests cover this pattern, update documentation as needed, and avoid relying on parentheses-based parsing in this context. If similar patterns appear elsewhere, consider expanding the rule with a focused pattern to cover related TS files in the binding attribute behaviors area.
Applied to files:
packages/bind/spec/bindingAttributeBehaviors.ts
🧬 Code graph analysis (11)
packages/bind/spec/bindingDependencyBehaviors.ts (4)
packages/binding.if/src/ConditionalBindingHandler.ts (1)
bindingContext(39-41)packages/binding.if/src/ifUnless.ts (1)
bindingContext(20-30)packages/binding.if/src/with.ts (1)
bindingContext(27-34)packages/bind/src/bindingContext.ts (2)
bindingContext(56-64)bindingContext(68-161)
packages/provider.native/src/NativeProvider.ts (1)
packages/bind/src/bindingContext.ts (1)
BindingContext(25-53)
packages/binding.template/spec/foreachBehaviors.ts (1)
packages/observable/src/observableArray.ts (2)
ObservableArray(137-141)observableArray(143-157)
packages/binding.core/spec/textBehaviors.ts (3)
packages/binding.if/src/ConditionalBindingHandler.ts (1)
bindingContext(39-41)packages/binding.if/src/ifUnless.ts (1)
bindingContext(20-30)packages/binding.if/src/with.ts (1)
bindingContext(27-34)
packages/binding.foreach/src/foreach.ts (2)
packages/bind/src/BindingHandler.ts (1)
AllBindings(10-17)packages/bind/src/index.ts (1)
AllBindings(7-7)
packages/provider.bindingstring/src/BindingStringProvider.ts (3)
packages/utils.parser/src/Arguments.ts (1)
Node(20-22)packages/utils.parser/src/Node.ts (1)
Node(7-123)packages/utils.parser/src/Parameters.ts (1)
Node(33-35)
packages/binding.template/src/templateSources.ts (3)
packages/utils/src/dom/info.ts (1)
tagNameLower(35-40)packages/utils/src/dom/html.ts (2)
setHtml(160-206)parseHtmlForTemplateNodes(147-150)packages/utils/src/index.ts (1)
domData(33-33)
packages/bind/src/applyBindings.ts (1)
packages/utils.parser/src/Parser.ts (1)
error(102-107)
packages/observable/src/dependencyDetection.ts (3)
packages/computed/src/computed.ts (1)
getDependencies(262-271)packages/observable/src/subscribable.ts (1)
Subscribable(55-59)packages/observable/src/index.ts (1)
Subscribable(16-16)
packages/binding.component/spec/componentBindingBehaviors.ts (2)
packages/observable/src/observableArray.ts (2)
ObservableArray(137-141)observableArray(143-157)packages/observable/src/index.ts (2)
ObservableArray(14-14)observableArray(10-10)
packages/binding.foreach/spec/eachBehavior.ts (1)
packages/observable/src/observableArray.ts (2)
ObservableArray(137-141)observableArray(143-157)
🪛 ast-grep (0.40.0)
packages/binding.template/src/templateSources.ts
[warning] 153-153: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: templateData.textData = templateData.containerData.innerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
⏰ 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 (59)
packages/provider/spec/providerBehaviors.ts (1)
19-19: LGTM!The addition of the
overridemodifier correctly satisfies the newnoImplicitOverridecompiler option and explicitly documents thatFOR_NODE_TYPESoverrides a base class member.packages/observable/spec/observableArrayBehaviors.ts (1)
236-249: LGTM! Explicit return satisfies strict TypeScript requirements.The addition of
return falseon line 245 ensures the predicate function explicitly returns a boolean value in all code paths. This change is necessary to comply with thenoImplicitReturnsTypeScript compiler option enabled in this PR, improving type safety without altering behavior.packages/computed/src/computed.ts (1)
282-298: LGTM! Explicit return statement required for strict TypeScript.The addition of
return falseat line 297 correctly ensures all code paths return a boolean value, which is required by thenoImplicitReturnscompiler option. The function logic remains sound: it returnstrueif any dependency has changed, and now explicitly returnsfalsewhen no changes are detected.packages/binding.template/spec/foreachBehaviors.ts (3)
314-324: LGTM! Explicit return satisfiesnoImplicitReturns.The explicit
return undefinedcorrectly handles the case where no preprocessing is needed, satisfying TypeScript'snoImplicitReturnscompiler option.
381-381: LGTM! Explicit generic type parameter added.The explicit
<any>type parameter correctly satisfies TypeScript's stricter type requirements. SinceObservableArraydefaults toany, the current type annotation is consistent with the function call.
943-958: LGTM! Explicit return satisfiesnoImplicitReturns.The explicit
return undefinedcorrectly handles the default case where no node preprocessing is performed, satisfying TypeScript'snoImplicitReturnscompiler option.packages/binding.component/spec/componentBindingBehaviors.ts (2)
881-881: LGTM! Consistent override declarations.The addition of the
overridekeyword to all statictemplategetters is correct and aligns with enablingnoImplicitOverride: true. These changes make the inheritance hierarchy explicit without altering runtime behavior.Also applies to: 896-896, 923-923, 942-942, 954-954, 993-993, 1020-1020, 1038-1038, 1059-1059, 1080-1080, 1102-1102, 1123-1123, 1147-1147, 1168-1168, 1191-1191, 1213-1213, 1240-1240, 1267-1267, 1301-1301, 1324-1324, 1353-1353, 1396-1396
1343-1343: LGTM! Explicit type parameters improve type safety.Adding explicit type parameters to
observableArraycalls clarifies the intent and ensures type safety under stricter TypeScript settings.Also applies to: 1382-1382
packages/binding.if/src/ifUnless.ts (2)
20-20: LGTM! Override keywords correctly applied.The
overridemodifiers onbindingContextandrenderStatuscorrectly indicate these members override parent class implementations, satisfying thenoImplicitOverride: truecompiler option enabled in this PR.Also applies to: 32-32
47-49: LGTM! Override correctly applied.The
overridemodifier onshouldDisplayIfcorrectly indicates this method overrides the parent class (IfBindingHandler) implementation, and the logic properly negates the parent's return value.packages/binding.core/spec/optionsBehaviors.ts (1)
178-178: LGTM! Explicit generic type improves type safety.The union type
string | numberis appropriate since the test exercises both string values (lines 189, 194) and number values (line 204). This explicit type annotation aligns well with the PR objective of enabling stricter TypeScript checking.packages/observable/src/dependencyDetection.ts (1)
55-74: LGTM! Explicit returns improve clarity.The explicit
return undefinedstatements in these three functions correctly satisfy thenoImplicitReturnsTypeScript strictness option. The changes align with the existing return type declarations and make the early-exit behavior more explicit.packages/provider/src/Provider.ts (1)
104-137: LGTM! Override modifiers correctly applied.The
overridemodifiers on these four LegacyProvider members correctly indicate that they override base Provider class members. The changes satisfy thenoImplicitOverridestrictness option without altering behavior.packages/binding.core/src/value.ts (1)
12-12: LGTM! Field override properly declared.The
overridemodifier correctly indicates that$elementoverrides a base class member, satisfying thenoImplicitOverriderequirement.packages/utils.jsx/spec/jsxBehaviors.ts (1)
31-34: LGTM! Test override properly annotated.The
overridemodifier correctly indicates thatdetachAndDisposeoverrides the base JsxObserver method, aligning with thenoImplicitOverridestrictness requirement.packages/utils.jsx/src/JsxObserver.ts (1)
135-153: LGTM! Override modifier correctly applied.The
overridemodifier properly indicates that thisdisposemethod overrides the base LifeCycle class method, satisfying thenoImplicitOverriderequirement. The call tosuper.dispose()confirms this is an override.packages/binding.component/src/slotBinding.ts (1)
81-83: LGTM! Static getter override properly declared.The
overridemodifier correctly indicates that this static getter overrides a base class member, satisfying thenoImplicitOverriderequirement.tsconfig.json (1)
18-23: LGTM! Stricter TypeScript configuration improves type safety.Enabling these three compiler options enhances type safety across the codebase:
noImplicitOverride: Ensures explicit override declarations, preventing accidental overridesnoImplicitReturns: Requires explicit returns in all code paths, making control flow clearerstrictFunctionTypes: Enables stricter checking of function parameter typesThese changes align with TypeScript best practices and improve maintainability.
packages/provider.bindingstring/src/BindingStringProvider.ts (1)
47-54: LGTM! Override modifier correctly applied.The
overridemodifier properly indicates thatgetBindingAccessorsoverrides the base Provider class method, satisfying thenoImplicitOverriderequirement without changing behavior.packages/provider.mustache/src/TextMustacheProvider.ts (1)
11-13: LGTM!The
overridemodifiers are correctly applied toFOR_NODE_TYPESgetter andpreprocessNodemethod, which override their respective base class members fromProvider. This aligns with thenoImplicitOverridecompiler option being enabled.Also applies to: 59-61
packages/provider.virtual/src/VirtualProvider.ts (1)
8-10: LGTM!The changes correctly implement the stricter TypeScript configuration:
overridemodifiers properly applied to all overridden members (FOR_NODE_TYPES,preprocessNode,getBindingString,nodeHasBindings)- Explicit return statements added at lines 31, 44, and 51 to satisfy
noImplicitReturns- Explicit return types added for clarity and type safety
Also applies to: 16-32, 40-52
packages/binding.core/src/descendantsComplete.ts (1)
13-15: LGTM!The
static overridemodifier is correctly applied to theallowVirtualElementsstatic getter, which overrides the base class implementation fromBindingHandler.packages/binding.if/src/ConditionalBindingHandler.ts (1)
142-147: LGTM!The
overridemodifiers are correctly applied to bothcontrolsDescendants(instance getter) andallowVirtualElements(static getter), properly indicating they override members from theAsyncBindingHandlerbase class.packages/binding.foreach/spec/eachBehavior.ts (1)
230-230: LGTM!Explicit generic type arguments are correctly added to
observableArraycalls:
- Line 230:
observableArray<number>([])— matches the numeric values assigned later- Line 755:
observableArray<any>([])— reasonable for a test with mixed content- Line 906:
observableArray<string>([])— matches the string value pushedThese changes satisfy
strictFunctionTypesby providing explicit type information.Also applies to: 755-755, 906-906
packages/provider.attr/src/AttributeProvider.ts (1)
10-10: LGTM! TypeScript override modifiers correctly applied.The
overridemodifiers onFOR_NODE_TYPES,nodeHasBindings, andgetBindingAccessorscorrectly signal that these members override base class implementations, aligning with the stricternoImplicitOverrideTypeScript option enabled in this PR.Also applies to: 25-25, 29-29
packages/provider.bindingstring/spec/BindingStringProviderBehaviors.ts (1)
9-9: LGTM! Test class correctly annotated with override modifiers.The
overridekeywords on the test class methods correctly indicate overrides of the parentBindingStringProviderclass, ensuring type safety in the test suite.Also applies to: 12-12
packages/binding.foreach/src/foreach.ts (2)
100-100: LGTM! Override modifiers correctly applied.The
overridekeywords onallBindings,dispose,controlsDescendants, andallowVirtualElementscorrectly mark these as overrides of base class members, ensuring type safety under the stricternoImplicitOverridecompiler option.Also applies to: 173-173, 575-578
388-394: LGTM! Explicit return satisfies noImplicitReturns.The explicit
return nullon Line 393 ensures all code paths inactiveChildElementhave a return statement, satisfying thenoImplicitReturnsTypeScript option. Previously, this path would have implicitly returnedundefined.packages/binding.component/src/componentBinding.ts (1)
186-186: LGTM! Override modifiers correctly applied.The
overridekeywords ondispose,controlsDescendants, andallowVirtualElementsproperly indicate that these members override base class implementations inDescendantBindingHandler.Also applies to: 191-194
packages/provider.databind/src/DataBindProvider.ts (2)
6-6: LGTM! Override modifiers correctly applied.The
overridekeywords onFOR_NODE_TYPES,getBindingString, andnodeHasBindingscorrectly mark these as overrides of parentBindingStringProvidermembers.Also applies to: 14-14, 21-21
14-19: LGTM! Explicit returns improve type safety.The explicit
return null(Line 18) andreturn false(Line 25) ensure all code paths have returns, satisfyingnoImplicitReturns. This also narrows the return types fromstring | null | undefinedtostring | nullandboolean | undefinedtoboolean, improving type precision.Also applies to: 21-26
packages/provider.native/src/NativeProvider.ts (1)
16-21: LGTM! Override modifiers correctly applied.The
overridekeywords onFOR_NODE_TYPES,preemptive,nodeHasBindings,preprocessNode, andgetBindingAccessorscorrectly indicate that these members override baseProviderclass implementations, ensuring proper type checking undernoImplicitOverride.Also applies to: 23-23, 34-36, 52-58
packages/provider.mustache/src/AttributeMustacheProvider.ts (1)
24-24: LGTM! Override modifiers correctly applied.The
overridekeywords onFOR_NODE_TYPES,nodeHasBindings, andgetBindingAccessorsproperly indicate overrides of baseProviderclass members, ensuring type safety under the stricter compiler options.Also applies to: 45-45, 95-95
packages/provider.mustache/spec/attributeInterpolationSpec.ts (1)
191-191: LGTM! Test class correctly annotated with override modifier.The
overridekeyword on theattributeBindingmethod correctly indicates that the test classKOAttris overriding the parentAttributeMustacheProvidermethod, maintaining type safety in the test suite.packages/bind/src/LegacyBindingHandler.ts (1)
55-55: LGTM! Override modifiers correctly added.The
overridemodifiers have been properly added to all overridden members to satisfy thenoImplicitOverrideTypeScript configuration.Also applies to: 82-82, 90-90, 98-98, 104-104
packages/provider.multi/src/MultiProvider.ts (1)
12-12: LGTM! Override modifiers correctly added.All
overridemodifiers have been properly added to methods that override the baseProviderclass.Also applies to: 25-25, 47-47, 51-51, 73-73
packages/provider.component/src/ComponentProvider.ts (1)
14-14: LGTM! TypeScript strict configuration compliance achieved.The changes correctly:
- Add
overridemodifiers to overridden Provider methods- Add explicit
returnstatements to satisfynoImplicitReturnsAlso applies to: 22-40, 42-44, 46-53, 55-68
packages/binding.core/src/textInput.ts (1)
18-18: LGTM! Override modifiers correctly added.The
overridemodifiers on the property and methods are correctly applied for TypeScript strict configuration compliance.Also applies to: 131-134, 197-199, 203-207
packages/binding.template/src/templateSources.ts (1)
57-137: LGTM! Successful refactor from function-based to class-based implementation.The conversion of
domElementandanonymousTemplatefrom prototype-based functions to TypeScript classes is well-executed:
- Proper method overloads for
text(),data(), andnodes()- TypeScript
overridemodifiers correctly applied inanonymousTemplate- Implementation logic preserved
Regarding the static analysis warning about innerHTML on line 153: This is a false positive—the code reads
innerHTMLto retrieve template data, it doesn't write user input to it.Also applies to: 143-162
packages/bind/src/DescendantBindingHandler.ts (1)
12-14: LGTM! Override modifier correctly added.packages/binding.template/src/foreach.ts (1)
10-34: LGTM! Override modifier correctly added.packages/bind/spec/bindingDependencyBehaviors.ts (1)
291-300: LGTM! Override modifier correctly added in test.packages/bind/src/BindingHandler.ts (1)
120-122: LGTM! Correct use of override modifier.The
overridekeyword correctly indicates thatAsyncBindingHandleris overriding thebindingCompletedgetter from the baseBindingHandlerclass (defined at line 92).packages/binding.if/src/else.ts (2)
14-16: LGTM! Correct override declaration.The
overridemodifier correctly indicates thatshouldDisplayIf()overrides the base class method fromIfBindingHandler.
22-27: LGTM! Correct override declaration.The
overridemodifier correctly indicates that theelseChainIsAlreadySatisfiedgetter overrides the base class property fromIfBindingHandler.packages/binding.if/src/with.ts (2)
27-34: LGTM! Correct override declaration.The
overridemodifier correctly indicates thatbindingContextoverrides the base getter fromConditionalBindingHandler.
36-39: LGTM! Correct override declaration.The
overridemodifier correctly indicates thatrenderStatus()overrides the base class method fromConditionalBindingHandler.packages/bind/spec/nodePreprocessingBehaviors.ts (3)
36-48: LGTM! Explicit return satisfies noImplicitReturns.The explicit
return undefinedat line 47 correctly satisfies thenoImplicitReturnsTypeScript compiler option for the code path where no node replacement occurs.
60-82: LGTM! Correct override usage and explicit return.Line 61 correctly uses the
overridemodifier for thepreprocessNodemethod, and line 80 adds an explicitreturn undefinedto satisfynoImplicitReturns.
97-109: LGTM! Correct override usage and explicit return.Line 98 correctly uses the
overridemodifier for thepreprocessNodemethod, and line 107 adds an explicitreturn undefinedto satisfynoImplicitReturns.packages/binding.template/src/templating.ts (1)
478-483: LGTM! Correct override declarations.Both
overridemodifiers are correctly applied:
- Line 478:
controlsDescendantsgetter overrides the base class property fromBindingHandler- Line 481: Static
allowVirtualElementsgetter overrides the base class static property fromBindingHandlerpackages/bind/spec/bindingAttributeBehaviors.ts (1)
828-853: LGTM! Correct override declarations in TestProvider.All three
overridemodifiers are correctly applied to methods that override the baseProviderclass:
- Line 829:
FOR_NODE_TYPESgetter- Line 832:
nodeHasBindingsmethod- Line 842:
getBindingAccessorsmethodpackages/binding.core/spec/textBehaviors.ts (1)
96-122: LGTM! Correct override declarations in TestProvider.All three
overridemodifiers are correctly applied to methods that override the baseProviderclass:
- Line 97:
FOR_NODE_TYPESgetter- Line 100:
nodeHasBindingsmethod- Line 110:
getBindingAccessorsmethodThis follows the same pattern as other test providers in the codebase.
packages/bind/spec/bindingCompletionPromiseBehavior.ts (3)
19-26: LGTM! Correct override declarations in SyncBinding.Both
overridemodifiers are correctly applied:
- Line 20:
bindingCompletedgetter overridesBindingHandler.bindingCompleted- Line 23: Static
allowVirtualElementsgetter overridesBindingHandler.allowVirtualElements
28-35: LGTM! Correct override declarations in AsyncBinding.Both
overridemodifiers are correctly applied:
- Line 29:
bindingCompletedgetter overridesBindingHandler.bindingCompleted- Line 32: Static
allowVirtualElementsgetter overridesBindingHandler.allowVirtualElements
37-51: LGTM! Correct override declarations in AsyncContainerBinding.Both
overridemodifiers are correctly applied:
- Line 45:
controlsDescendantsgetter overridesBindingHandler.controlsDescendants- Line 48:
bindingCompletedgetter overridesBindingHandler.bindingCompletedpackages/binding.template/spec/templatingBehaviors.ts (3)
479-498: LGTM! TypeScript override modifiers correctly applied.The
overridemodifiers have been properly added to theFOR_NODE_TYPESgetter,nodeHasBindings, andgetBindingAccessorsmethods, ensuring compliance with thenoImplicitOverridecompiler option. No logic changes were made.
537-551: LGTM! Override modifiers correctly applied in skipped test.The
overridemodifiers have been properly added to the test provider methods within this skipped test. The changes are consistent with the project-wide TypeScript strictness improvements.
1319-1326: LGTM! Override modifier correctly applied to preprocessNode.The
overridemodifier has been properly added to thepreprocessNodemethod, ensuring type safety and compliance with the stricter TypeScript configuration.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/binding.template/src/templateSources.ts (1)
61-76: Constructor lacks runtime validation despite type annotation.While the constructor parameter is typed as
Element, the child classanonymousTemplateacceptselement?: any(line 140), which bypasses this type safety. Additionally, runtime validation would provide defense in depth for callers using type assertions or whenstrictNullChecksis not fully enforced throughout the codebase.If a null or undefined element reaches this constructor, methods like
text(),data(), andnodes()will fail when accessingthis.domElement.🔎 Proposed fix: Add runtime validation
constructor(element: Element) { + if (!element) { + throw new Error('Template element must not be null or undefined') + } this.domElement = element let tagNameLower = tagNameLowerFn(element)Note: This issue relates to a previous review comment about null handling, though the code structure has changed since that review.
🧹 Nitpick comments (3)
packages/utils/src/options.ts (1)
120-123: Consider function overloads for better type safety.The return type
typeof eapplies even whenthrowsistrueand the function never returns. TypeScript's control flow can't distinguish between the throwing case (which should benever) and the non-throwing case (which should betypeof e).Additionally, the parameter type
anydisables type checking—consider usingunknowninstead.🔎 Proposed type-safe implementation with overloads
- onError(e: any, throws: boolean = true): typeof e { + onError(e: unknown, throws: true): never; + onError(e: unknown, throws?: false): typeof e; + onError(e: unknown, throws: boolean = true): typeof e | never { if (throws) throw e return e }This makes the return type context-aware:
- When
throwsistrue(or default), return type isnever- When
throwsis explicitlyfalse, return type istypeof epackages/binding.template/src/templateSources.ts (2)
120-120: Remove unnecessary type cast.The cast
(this as any)['text']()is unnecessary sincetext()is a defined method on this class. The same issue occurs on line 123.🔎 Proposed fix
- const text = (this as any)['text']() + const text = this.text()And on line 123:
- ;(this as any)['text']('') // clear the text from the node + this.text('') // clear the text from the node
154-154: Prefer parameter name overargumentsarray.Using
arguments[0]is less readable than using the parameter name directly. Consider destructuring or naming the parameter.🔎 Proposed fix
- override text(/* valueToWrite */): string | void { + override text(valueToWrite?: string): string | void { 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 }) } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/bind/src/applyBindings.ts(5 hunks)packages/binding.core/src/textInput.ts(4 hunks)packages/binding.template/src/templateEngine.ts(1 hunks)packages/binding.template/src/templateSources.ts(1 hunks)packages/utils/src/options.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/bind/src/applyBindings.ts
- packages/binding.template/src/templateEngine.ts
- packages/binding.core/src/textInput.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/binding.template/src/templateSources.ts (3)
packages/utils/src/dom/info.ts (1)
tagNameLower(35-40)packages/utils/src/dom/html.ts (2)
setHtml(160-206)parseHtmlForTemplateNodes(147-150)packages/utils/src/index.ts (1)
domData(33-33)
🪛 ast-grep (0.40.0)
packages/binding.template/src/templateSources.ts
[warning] 149-149: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: templateData.textData = templateData.containerData.innerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
⏰ 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 (1)
packages/binding.template/src/templateSources.ts (1)
149-149: Static analysis XSS warning is a false positive.The static analysis tool flagged line 149 for potential XSS vulnerability, but this is reading
innerHTMLfrom an existing DOM node to serialize it, not writing unsanitized user input to the DOM. This is a safe operation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/binding.template/src/templateSources.ts (5)
78-93: Consider leveraging TypeScript's parameter discrimination instead ofarguments.length.While the overloads are correctly declared, checking
arguments.lengthat runtime is less idiomatic in TypeScript. You can rely on the presence of the parameter itself.🔎 Cleaner implementation using parameter presence
text(): string text(valueToWrite: string): void -text(valueToWrite?: string): string | void { +text(valueToWrite?: string): string | void { const elemContentsProperty = this.templateType === templateScript ? 'text' : this.templateType === templateTextArea ? 'value' : 'innerHTML' - if (arguments.length == 0) { + if (valueToWrite === undefined) { return this.domElement[elemContentsProperty] } else { if (elemContentsProperty === 'innerHTML') { setHtml(this.domElement, valueToWrite!) } else { this.domElement[elemContentsProperty] = valueToWrite } } }
95-101: Samearguments.lengthpattern here—consider checking parameter presence instead.Consistent with the
text()method, you can simplify this by checking ifvalueToWrite === undefined.🔎 Suggested refactor
data<T = any>(key: string, valueToWrite?: T): T | void { - if (arguments.length === 1) { + if (valueToWrite === undefined) { return domData.get(this.domElement, dataDomDataPrefix + key) } else { domData.set(this.domElement, dataDomDataPrefix + key, valueToWrite) } }
105-132: Simplify parameter check and remove unnecessaryanycasts.Two issues here:
arguments.lengthcan be replaced with parameter presence check- Lines 120 and 123 use
(this as any)['text']whenthis.text()is perfectly type-safe🔎 Proposed improvements
nodes(): Node nodes(valueToWrite: Node): void -nodes(valueToWrite?: any): Node | void { +nodes(valueToWrite?: Node): Node | void { const element = this.domElement - if (arguments.length == 0) { + if (valueToWrite === undefined) { const templateData = getTemplateDomData(element) let nodes = templateData.containerData || (this.templateType === templateTemplate ? (element as HTMLTemplateElement).content : this.templateType === templateElement ? element : undefined) if (!nodes || templateData.alwaysCheckText) { // If the template is associated with an element that stores the template as text, // parse and cache the nodes whenever there's new text content available. This allows // the user to update the template content by updating the text of template node. - const text = (this as any)['text']() + const text = this.text() if (text) { nodes = parseHtmlForTemplateNodes(text, element.ownerDocument) - ;(this as any)['text']('') // clear the text from the node + this.text('') // clear the text from the node setTemplateDomData(element, { containerData: nodes, alwaysCheckText: true }) } } return nodes } else { setTemplateDomData(element, { containerData: valueToWrite }) } }
140-142: Constructor can be omitted.When a subclass constructor only calls
super()with the same parameters, TypeScript doesn't require an explicit constructor declaration.🔎 Simplification
export class anonymousTemplate extends domElement { - constructor(element: Element) { - super(element) - } - override text(): string override text(valueToWrite: string): void
144-157: Prefer parameter presence check overarguments.length.Same pattern as the parent class—checking
arguments.lengthis less idiomatic than checking forundefined.🔎 Suggested improvement
override text(): string override text(valueToWrite: string): void -override text(/* valueToWrite */): string | void { - if (arguments.length == 0) { +override text(valueToWrite?: string): string | void { + if (valueToWrite === undefined) { 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 }) } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/binding.template/src/templateEngine.ts(1 hunks)packages/binding.template/src/templateSources.ts(1 hunks)
🧰 Additional context used
🪛 ast-grep (0.40.0)
packages/binding.template/src/templateSources.ts
[warning] 149-149: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: templateData.textData = templateData.containerData.innerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
⏰ 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)
packages/binding.template/src/templateSources.ts (1)
61-76: Constructor implementation looks solid.The constructor properly initializes both
domElementandtemplateTypebased on the element's tag name and template capabilities. The type narrowing for HTMLTemplateElement is appropriate.packages/binding.template/src/templateEngine.ts (2)
104-104: Good refactor: using direct method call.Changing from
this['makeTemplateSource']tothis.makeTemplateSourceimproves type safety and readability.
87-87: Error handling pattern is correct and intentional.The
options.onError()function (from@tko/utils) is designed to return the error when called withthrows: false, allowing the caller to throw it. The signatureonError(e: any, throws: boolean = true): typeof eensures it returns the error object itself. The patternthrow options.onError(new Error(...), false)is valid and intentional—onErrorreturns the error without throwing (sincefalseis passed), and the outerthrowstatement handles the throw. No changes needed.
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 (1)
packages/binding.template/src/templateSources.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/binding.template/src/templateSources.ts (2)
packages/utils/src/dom/info.ts (1)
tagNameLower(35-40)packages/utils/src/dom/html.ts (2)
setHtml(160-206)parseHtmlForTemplateNodes(147-150)
🪛 ast-grep (0.40.0)
packages/binding.template/src/templateSources.ts
[warning] 149-149: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: templateData.textData = templateData.containerData.innerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
⏰ 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, off)
🔇 Additional comments (3)
packages/binding.template/src/templateSources.ts (3)
57-76: LGTM! Class-based refactor with proper constructor typing.The domElement class correctly implements the TemplateSource interface with a non-nullable Element constructor parameter. The templateType initialization logic properly handles different HTML element types (script, textarea, template, element), and the necessary type casts are appropriate.
78-101: LGTM! Method overloads properly typed.The text() and data() methods correctly implement getter/setter overloads with proper type signatures. The use of arguments.length to distinguish calls is appropriate for overload implementations, and both methods align with the stricter TypeScript configuration (explicit return types matching noImplicitReturns).
139-158: LGTM! Properly uses override keywords.The anonymousTemplate subclass correctly uses the override keyword on all text() method overloads (lines 144-146), aligning with the noImplicitOverride TypeScript configuration. The constructor properly matches the parent signature with a non-nullable Element parameter.
Regarding the static analysis warning on line 149: this is a false positive. The code is reading innerHTML (
templateData.containerData.innerHTML), not writing to it. XSS vulnerabilities arise from writing unsanitized content to innerHTML, but reading is safe.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bind/src/applyBindings.ts (1)
475-481: Fix variable scope issue causing ReferenceError.At line 480,
bindingContextis referenced but it's only declared inside the if block at line 476. WhenrootNode.nodeTypeis neitherELEMENT_NODEnorCOMMENT_NODE(e.g.,TEXT_NODE= 3), execution falls through to line 480 wherebindingContextis undefined, causing a ReferenceError.🔎 Proposed fix
export function applyBindingsToDescendants<T = any>( viewModelOrBindingContext: T | BindingContext<T>, rootNode: Node ): BindingResult { const asyncBindingsApplied = new Set() + const bindingContext = getBindingContext(viewModelOrBindingContext) if (rootNode.nodeType === Node.ELEMENT_NODE || rootNode.nodeType === Node.COMMENT_NODE) { - const bindingContext = getBindingContext(viewModelOrBindingContext) applyBindingsToDescendantsInternal(bindingContext, rootNode, asyncBindingsApplied) return new BindingResult({ asyncBindingsApplied, rootNode, bindingContext }) } return new BindingResult({ asyncBindingsApplied, rootNode, bindingContext }) }
♻️ Duplicate comments (1)
packages/binding.template/src/templateSources.ts (1)
107-136: Type signature violation: nodes() can return undefined.The overload signature at line 107 declares
nodes(): Node, but the implementation can returnundefinedwhen:
templateData.containerDatais falsy and the element type doesn't match template or element types (line 119)- The text content is falsy (lines 124-125)
In these cases, line 131 returns
nodeswhich may beundefined, violating the non-nullable return type and potentially causing runtime errors in callers expecting a validNode.🔎 Proposed fix to match signature with implementation
- nodes(): Node + nodes(): Node | undefined nodes(valueToWrite: Node): undefined - nodes(valueToWrite?: any): Node | undefined { + nodes(valueToWrite?: any): Node | undefined | void {Note: This also requires updating the
TemplateSourceinterface at line 45 to allowundefinedreturns, and auditing all callers to handle the undefined case safely.
🧹 Nitpick comments (4)
packages/utils/src/dom/html.ts (1)
220-220: Consider using Node.TEXT_NODE for consistency.For consistency with the pattern established on line 193, consider replacing the magic number
3withNode.TEXT_NODE.🔎 Proposed refactor
- if (!innerTextNode || innerTextNode.nodeType != 3 || virtualElements.nextSibling(innerTextNode)) { + if (!innerTextNode || innerTextNode.nodeType !== Node.TEXT_NODE || virtualElements.nextSibling(innerTextNode)) {Note: Also changed
!=to!==for strict equality, which is preferred in TypeScript.packages/utils/src/dom/disposal.ts (1)
14-15: Consider using DOM constants for consistency.For consistency with Line 62, consider replacing the magic numbers here with DOM constants (
Node.ELEMENT_NODE,Node.COMMENT_NODE,Node.DOCUMENT_NODE).🔎 Proposed refactor
-let cleanableNodeTypes = { 1: true, 8: true, 9: true } -let cleanableNodeTypesWithDescendants = { 1: true, 9: true } +let cleanableNodeTypes = { [Node.ELEMENT_NODE]: true, [Node.COMMENT_NODE]: true, [Node.DOCUMENT_NODE]: true } +let cleanableNodeTypesWithDescendants = { [Node.ELEMENT_NODE]: true, [Node.DOCUMENT_NODE]: true }builds/knockout/spec/defaultBindings/foreachBehaviors.js (1)
235-235: Consider usingNode.TEXT_NODEfor consistency.For consistency with the change on line 658, consider replacing the magic number
3with the named constantNode.TEXT_NODE:- if (node.nodeType === 3 && node.data.charAt(0) === "$") { + if (node.nodeType === Node.TEXT_NODE && node.data.charAt(0) === "$") {This would align with the broader pattern in this PR of replacing numeric nodeType checks with named constants.
packages/bind/src/applyBindings.ts (1)
522-529: LGTM with optional refinement suggestion.The error handling correctly addresses the read-only error concern by always creating a new Error instance. The
error.message || String(error)fallback ensures a message is always available.💡 Optional: Preserve original error name
For better debugging, consider preserving the original error type name:
const message = error.message || String(error) error = new Error(message) + if (error.name && error.name !== 'Error') { + error.name = error.name + } extend(error, spec)Wait, that won't work since we've already reassigned
error. Here's the correct approach:const message = error.message || String(error) - error = new Error(message) + const originalName = error.name || error.constructor?.name + error = new Error(message) + if (originalName && originalName !== 'Error') { + error.name = originalName + } extend(error, spec)This preserves diagnostic context when the original error was a
DOMExceptionor other error subclass.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
builds/knockout/spec/defaultBindings/foreachBehaviors.js(1 hunks)builds/knockout/spec/templatingBehaviors.js(1 hunks)packages/bind/docs/binding-preprocessing.md(1 hunks)packages/bind/src/applyBindings.ts(9 hunks)packages/bind/src/bindingContext.ts(1 hunks)packages/binding.if/src/ConditionalBindingHandler.ts(2 hunks)packages/binding.if/src/else.ts(2 hunks)packages/binding.template/helpers/dummyTemplateEngine.ts(1 hunks)packages/binding.template/spec/foreachBehaviors.ts(4 hunks)packages/binding.template/src/templateEngine.ts(1 hunks)packages/binding.template/src/templateSources.ts(1 hunks)packages/binding.template/src/templating.ts(2 hunks)packages/utils.jsx/src/JsxObserver.ts(4 hunks)packages/utils/src/dom/disposal.ts(1 hunks)packages/utils/src/dom/fixes.ts(2 hunks)packages/utils/src/dom/html.ts(1 hunks)packages/utils/src/dom/info.ts(2 hunks)packages/utils/src/dom/virtualElements.ts(3 hunks)packages/utils/src/memoization.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/binding.if/src/else.ts
- packages/binding.if/src/ConditionalBindingHandler.ts
- packages/binding.template/spec/foreachBehaviors.ts
🧰 Additional context used
🧬 Code graph analysis (4)
packages/binding.template/src/templating.ts (4)
packages/bind/src/bindingContext.ts (2)
bindingContext(56-64)bindingContext(68-161)packages/binding.if/src/ConditionalBindingHandler.ts (1)
bindingContext(39-41)packages/binding.if/src/ifUnless.ts (1)
bindingContext(20-30)packages/binding.if/src/with.ts (1)
bindingContext(27-34)
packages/binding.template/src/templateSources.ts (3)
packages/utils/src/dom/info.ts (1)
tagNameLower(35-40)packages/utils/src/dom/html.ts (2)
setHtml(160-206)parseHtmlForTemplateNodes(147-150)packages/utils/src/index.ts (1)
domData(33-33)
builds/knockout/spec/defaultBindings/foreachBehaviors.js (1)
tko.io/src/tko-io.js (2)
atNode(38-46)node(65-69)
packages/binding.template/helpers/dummyTemplateEngine.ts (1)
packages/binding.template/src/templateSources.ts (1)
anonymousTemplate(143-163)
🪛 ast-grep (0.40.0)
packages/binding.template/src/templateSources.ts
[warning] 153-153: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: templateData.textData = templateData.containerData.innerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
⏰ 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 (32)
packages/bind/docs/binding-preprocessing.md (1)
106-106: Improved documentation example with semantic constant.The documentation example now uses
Node.COMMENT_NODEinstead of the magic number8, making the code clearer and demonstrating best practices for DOM node type checking. This aligns well with the PR's goal of stricter type safety.packages/utils/src/dom/info.ts (1)
10-10: LGTM! Excellent refactoring to use named constants.Replacing magic numbers (1, 11) with named Node constants (
Node.ELEMENT_NODE,Node.DOCUMENT_FRAGMENT_NODE) significantly improves code readability and maintainability. The logic remains unchanged, and this aligns well with the PR's goal of improving code quality.Also applies to: 14-14, 46-46, 54-54
packages/utils/src/memoization.ts (1)
20-26: LGTM! Excellent refactoring to use DOM constants.Replacing magic numbers (8, 1) with
Node.COMMENT_NODEandNode.ELEMENT_NODEsignificantly improves code readability and maintainability while preserving the exact same behavior.packages/bind/src/bindingContext.ts (1)
243-243: LGTM! Excellent readability improvement.Replacing magic numbers with DOM standard constants (
Node.ELEMENT_NODEandNode.COMMENT_NODE) makes the code self-documenting and easier to maintain.builds/knockout/spec/templatingBehaviors.js (1)
27-27: LGTM! Improved code readability.Replacing magic numbers (1 and 8) with the semantic DOM constants
Node.ELEMENT_NODEandNode.COMMENT_NODEmakes the code intent immediately clear and aligns with best practices.packages/utils/src/dom/html.ts (1)
193-193: LGTM! Improved readability with semantic constant.Replacing the magic number
8withNode.COMMENT_NODEmakes the code more self-documenting and aligns with best practices for readability and maintainability.packages/utils/src/dom/fixes.ts (2)
25-25: Good refactoring: magic number replaced with named constant.The use of
Node.COMMENT_NODEinstead of the magic number8improves readability and makes the code self-documenting.
69-69: Good refactoring: magic number replaced with named constant and strict equality.The use of
Node.ELEMENT_NODEinstead of the magic number1improves readability. The change from loose (==) to strict (===) equality is also a best practice.packages/utils/src/dom/disposal.ts (1)
62-62: LGTM! Improved readability with DOM constant.Replacing the magic number
8withNode.COMMENT_NODEimproves code clarity and aligns with the PR's goal of better type safety and readability.packages/utils/src/dom/virtualElements.ts (3)
31-43: LGTM! Excellent refactoring to use DOM constants.Replacing magic numbers (8) with
Node.COMMENT_NODEsignificantly improves readability and maintainability. The change from loose equality (==) to strict equality (===) also aligns with TypeScript best practices.
231-248: LGTM! Improved clarity with explicit control flow.The use of
Node.COMMENT_NODEis consistent with the other changes, and the explicitelsebranch (lines 242-246) makes the logic clearer for handling non-comment nodes at depth 0.
257-286: LGTM! Consistent use of DOM element constant.Using
Node.ELEMENT_NODEinstead of the magic number 1 maintains consistency with the other refactoring in this file and improves code readability.packages/utils.jsx/src/JsxObserver.ts (3)
77-77: LGTM! Excellent refactoring to use DOM constants.Replacing the magic number
8withNode.COMMENT_NODEsignificantly improves code readability and maintainability. This aligns with best practices and makes the intent explicit.Also applies to: 126-126
135-135: LGTM! Correct override declaration.The
overridekeyword correctly indicates that this method overrides the parentLifeCycle.dispose()method. This satisfies thenoImplicitOverrideTypeScript option enabled in this PR.
242-242: LGTM! Clear and correct node type checking.Using
Node.ELEMENT_NODEandNode.COMMENT_NODEconstants instead of numeric literals makes the node type check explicit and easier to understand. The logic correctly identifies nodes suitable forapplyBindings.builds/knockout/spec/defaultBindings/foreachBehaviors.js (1)
657-665: LGTM! Improved readability with named constant.The change from numeric value
1toNode.ELEMENT_NODEenhances code clarity and aligns with the PR's goal of improving code consistency across the repository.packages/bind/src/applyBindings.ts (7)
35-35: LGTM! Error type strictness properly enforced.The type change from
anytoErroris appropriately supported by the error wrapping logic at lines 387-388, ensuring that non-Error thrown values are converted before being passed toreportBindingError.
146-146: LGTM! Named constant improves readability.Using
Node.ELEMENT_NODEinstead of the magic number1makes the code more self-documenting and maintainable.
331-331: LGTM! Signature aligns with stricter typing.The
Errorparameter type ensures type safety and consistency with theBindingErrorinterface.
344-344: LGTM! Named constant improves code clarity.Using
Node.COMMENT_NODEmakes the virtual element check more self-documenting.
387-388: LGTM! Proper error wrapping ensures type safety.The
instanceof Errorcheck with fallback wrapping ensures thatreportBindingErroralways receives a proper Error instance, even when catch blocks receive non-Error thrown values.
446-446: LGTM! Consistent use of named constants.
505-505: LGTM! Explicit type annotation improves clarity.The explicit
Errortype declaration aligns with the stricter TypeScript configuration and makes the code more self-documenting.packages/binding.template/helpers/dummyTemplateEngine.ts (1)
31-32: LGTM!The changes correctly replace magic numbers with DOM constants (
Node.ELEMENT_NODE,Node.COMMENT_NODE) and use strict equality checks. This improves code readability and aligns with the project-wide pattern of modernizing node-type checks.packages/binding.template/src/templating.ts (3)
107-109: LGTM!The node type check correctly uses DOM constants with strict equality, consistent with the broader modernization effort across the codebase.
112-114: LGTM!Consistent with the previous node type check at line 107. The modernized constant usage is correct.
478-483: LGTM!The
overridemodifiers correctly indicate that these getters override parent class members, which is required by thenoImplicitOverrideTypeScript setting enabled in this PR. This improves type safety by catching accidental naming mismatches.packages/binding.template/src/templateSources.ts (4)
82-97: LGTM!The method overloads and implementation are well-structured. The use of
setHtmlforinnerHTMLassignment (line 92) is a safer approach compared to direct property assignment.
99-105: LGTM!The generic type parameter and overload-style implementation correctly handle both getter and setter patterns. The logic is sound.
143-162: LGTM with a note on innerHTML usage.The class correctly extends
domElementwith proper type signatures, addressing the previous concern about type safety bypass. Theoverridekeywords correctly indicate overridden methods, aligning with the stricter TypeScript configuration.Regarding the static analysis warning at line 154 about
innerHTML: this is reading (not writing) fromcontainerData.innerHTMLto serialize template content. The data source is controlled (internal template storage), not unsanitized user input, so this usage is safe in this context.
61-80: Constructor correctly enforces non-nullable element parameter.The constructor signature
constructor(element: Element | Comment)prevents null or undefined from being passed, which addresses the previous concern about incomplete initialization. When strictNullChecks is true, null and undefined have their own distinct types and you'll get a type error if you try to use them where a concrete value is expected. All instantiation sites (templateEngine.ts lines 89, 92, 95 and dummyTemplateEngine.ts line 32) perform proper type narrowing through null checks and nodeType validation before construction, ensuring the type system prevents invalid calls at compile time.packages/binding.template/src/templateEngine.ts (1)
87-87: Document the purpose of thefalseparameter in error handling calls.The second parameter to
options.onError()controls error throwing behavior. Whenfalse, the handler returns the error without throwing, allowing the caller to explicitly throw it. This pattern enables custom error handlers to intercept errors while maintaining throw semantics required by the template engine.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/bind/src/applyBindings.ts (1)
522-529: Consider preserving the original error name.The error handling now correctly creates a fresh Error instance to avoid read-only issues. However, the original error's name is not preserved, which could lose useful context (e.g., "DOMException" becoming "Error").
🔎 Optional enhancement to preserve error type information
} catch (e: any) { // Read-only error e.g. a DOMEXception. spec.stack = error.stack - const message = error.message || String(error) error = new Error(message) + if (error.name && error.name !== 'Error') { + error.name = error.name + } extend(error, spec) }This preserves the original error type name for better diagnostics.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/bind/src/applyBindings.ts(10 hunks)packages/binding.component/src/componentBinding.ts(2 hunks)packages/binding.if/src/else.ts(2 hunks)packages/binding.template/src/templateEngine.ts(2 hunks)
🔇 Additional comments (11)
packages/binding.if/src/else.ts (3)
14-16: LGTM! Override keyword correctly applied.The explicit
overridemodifier correctly indicates this method overrides the parent class implementation, which is required by thenoImplicitOverrideTypeScript setting.
22-27: LGTM! Override keyword correctly applied.The explicit
overridemodifier on the getter is correct and consistent with the stricter TypeScript configuration.
33-33: LGTM! Excellent use of named constants.Replacing numeric literals (
1,8) withNode.ELEMENT_NODEandNode.COMMENT_NODEsignificantly improves code readability and maintainability. This is a best practice for working with DOM node types.Also applies to: 39-39
packages/binding.component/src/componentBinding.ts (2)
75-75: LGTM! Improved readability with DOM constant.Using
Node.ELEMENT_NODEinstead of the magic number1makes the code more readable and self-documenting.
186-196: All override modifiers are correctly applied.Verification confirms that the three overridden members exist in the parent class hierarchy with compatible signatures:
dispose()is inherited fromLifeCyclecontrolsDescendantsgetter is defined inBindingHandler(returnsfalseby default)allowVirtualElementsstatic getter is defined inBindingHandler(returnsfalseby default)The
overridekeywords properly enforce TypeScript'snoImplicitOverriderequirement.packages/bind/src/applyBindings.ts (3)
146-146: Excellent use of Node type constants!Replacing numeric literals with
Node.ELEMENT_NODEandNode.COMMENT_NODEimproves code readability and aligns with TypeScript best practices.Also applies to: 344-344, 446-446, 476-476, 496-496
331-342: Type-safe error handling signature.The explicit
Errortype for theerrorCapturedparameter, combined with the error wrapping logic at lines 387-388, ensures type safety and aligns with the stricter TypeScript configuration.
386-389: Proper error normalization in catch block.The error wrapping ensures that any thrown value (strings, objects, etc.) is normalized to an Error instance before being reported, which aligns with the stricter
Errortype onerrorCaptured.packages/binding.template/src/templateEngine.ts (3)
81-99: Correct type signature and comprehensive node handling.The
string | Nodesignature correctly matches the interface (line 49) and the implementation properly handles Element, DocumentFragment, and Comment node types with appropriate type assertions.
107-107: Improved method call syntax.Using dot notation (
this.makeTemplateSource) instead of bracket notation is more idiomatic and maintains better type safety.
87-87: The pattern is valid; no changes needed.The
options.onErrorfunction returns the error object when called withfalseas the second parameter, allowing the caller to throw it. This is intentional API design that provides a hook for error handling while maintaining throw semantics. The function signature confirms this:onError(e: any, throws: boolean = true): typeof ereturns the same type as the input error.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/binding.core/spec/textBehaviors.ts (1)
96-122: Override modifiers and Node constants look good.The
overridekeywords are correctly added to satisfynoImplicitOverride, and usingNode.TEXT_NODEinstead of the magic number3improves readability.Minor inconsistency: Line 98 still uses
document.ELEMENT_NODEwhile the new changes useNode.TEXT_NODE. For consistency, consider updating toNode.ELEMENT_NODE.🔎 Optional consistency fix
override get FOR_NODE_TYPES() { - return [document.ELEMENT_NODE] + return [Node.ELEMENT_NODE] }packages/bind/spec/bindingAttributeBehaviors.ts (1)
829-831: Override on getter and consider using constant in return value.The
overridekeyword is correctly applied to the getter. However, for consistency with the project-wide shift to named constants, consider replacing the return value[3]with[Node.TEXT_NODE].🔎 Proposed fix for consistency
override get FOR_NODE_TYPES() { - return [3] + return [Node.TEXT_NODE] }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
builds/knockout/helpers/jasmine.extensions.js(1 hunks)builds/knockout/spec/bindingAttributeBehaviors.js(2 hunks)builds/knockout/spec/defaultBindings/foreachBehaviors.js(2 hunks)builds/knockout/spec/defaultBindings/textBehaviors.js(1 hunks)builds/knockout/spec/nodePreprocessingBehaviors.js(2 hunks)packages/bind/spec/bindingAttributeBehaviors.ts(2 hunks)packages/bind/spec/nodePreprocessingBehaviors.ts(4 hunks)packages/binding.core/spec/textBehaviors.ts(1 hunks)packages/binding.template/spec/foreachBehaviors.ts(4 hunks)packages/binding.template/src/templateEngine.ts(2 hunks)packages/utils/helpers/jasmine-13-helper.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/binding.template/spec/foreachBehaviors.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-21T09:16:36.575Z
Learnt from: phillipc
Repo: knockout/tko PR: 221
File: packages/bind/spec/bindingAttributeBehaviors.ts:135-138
Timestamp: 2025-12-21T09:16:36.575Z
Learning: In the TKO framework, data-bind expressions may use naked arrow function syntax (zero-parameter arrows written without parentheses), e.g., data-bind='click: => was_clicked(true)' or lambdaLiteral: => null. This is valid TKO syntax, though not standard JavaScript. For code reviews, treat this as an allowed syntax specific to TKO in the analyzed file. Ensure tests cover this pattern, update documentation as needed, and avoid relying on parentheses-based parsing in this context. If similar patterns appear elsewhere, consider expanding the rule with a focused pattern to cover related TS files in the binding attribute behaviors area.
Applied to files:
packages/bind/spec/bindingAttributeBehaviors.ts
🧬 Code graph analysis (2)
builds/knockout/spec/bindingAttributeBehaviors.js (1)
tko.io/src/tko-io.js (1)
atNode(38-46)
packages/binding.template/src/templateEngine.ts (1)
packages/binding.template/src/templateSources.ts (3)
TemplateSource(35-46)domElement(57-137)anonymousTemplate(143-163)
⏰ 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 (22)
builds/knockout/spec/defaultBindings/textBehaviors.js (1)
64-64: LGTM! Good readability improvement.Replacing the magic number
3withNode.TEXT_NODEmakes the code self-documenting and aligns with the PR's modernization goals.packages/utils/helpers/jasmine-13-helper.ts (1)
65-65: LGTM! Improved readability with named constant.Replacing the numeric literal
3withNode.TEXT_NODEand using strict equality makes the code more self-documenting and maintainable.builds/knockout/helpers/jasmine.extensions.js (1)
46-48: LGTM! Improved readability with named constant.The change from a numeric literal to
Node.TEXT_NODEmakes the code more maintainable and self-documenting, while the strict equality operator (===) is better practice.builds/knockout/spec/defaultBindings/foreachBehaviors.js (2)
235-235: Excellent use of Node.TEXT_NODE constant.Replacing the magic number
3withNode.TEXT_NODEsignificantly improves code readability and maintainability. This makes the intent explicit and aligns with modern best practices.
658-664: Good refactor using Node.ELEMENT_NODE constant.The change from
elem.nodeType === 1toelem.nodeType === Node.ELEMENT_NODEimproves clarity. The test correctly implements the scenario described in the comment at line 647, where element nodes are removed asynchronously (for animations) while non-element nodes are removed immediately.builds/knockout/spec/nodePreprocessingBehaviors.js (2)
45-45: Good use of DOM constant.Replacing the magic number
3withNode.TEXT_NODEimproves code readability and aligns with the project-wide standardization on named DOM constants.
110-110: Consistent with the pattern established above.Same improvement using
Node.TEXT_NODEinstead of the numeric literal.packages/bind/spec/nodePreprocessingBehaviors.ts (5)
47-47: Explicit return satisfiesnoImplicitReturns.Adding
return undefinedensures all code paths have an explicit return, which is required by the new stricter TypeScript configuration.
61-64: Proper use ofoverrideand DOM constant.The
overridekeyword correctly signals thatpreprocessNodeoverrides a base class method (required bynoImplicitOverride), andNode.TEXT_NODEreplaces the magic number for better readability.
80-80: Explicit return path.Consistent with the pattern established for satisfying
noImplicitReturns.
98-99: Override and DOM constant applied consistently.Same pattern as the previous
TestProviderclass -overridekeyword andNode.TEXT_NODEconstant applied correctly.
107-107: Explicit return for all code paths.Completes the pattern ensuring all branches return explicitly.
packages/bind/spec/bindingAttributeBehaviors.ts (3)
612-614: DOM constant improves clarity.Using
Node.TEXT_NODEmakes the intent clear when checking if a node is a text node while iterating virtual children.
832-840: Override and DOM constant applied correctly.The
overridekeyword onnodeHasBindingsand the use ofNode.TEXT_NODEfor the type check are both appropriate.
842-852: Proper override and type check.The
overridekeyword ongetBindingAccessorsis correct, andNode.TEXT_NODEis used consistently for the node type comparison.builds/knockout/spec/bindingAttributeBehaviors.js (3)
393-395: DOM constant for text node check.Using
Node.TEXT_NODEinstead of the magic number3improves readability when iterating virtual children.
561-564: Consistent use of DOM constant.The
Node.TEXT_NODEconstant is correctly used in thenodeHasBindingsfunction for checking text nodes.
569-576: DOM constant in getBindingAccessors.Completes the pattern of using
Node.TEXT_NODEthroughout this test provider implementation.packages/binding.template/src/templateEngine.ts (4)
81-81: LGTM: Type signature correctly matches interface.The method signature with
string | Nodeparameter andTemplateSourcereturn type is consistent with the interface declaration at line 49.
90-95: Good: Explicit Node type constants improve readability.Replacing numeric literals with
Node.ELEMENT_NODEandNode.COMMENT_NODEmakes the code more maintainable and self-documenting. The type casts toElementandCommentare correctly guarded by the nodeType checks and match theanonymousTemplateconstructor signature.
107-107: Good: Dot notation improves type safety.Replacing bracket notation
this['makeTemplateSource']with standard dot notationthis.makeTemplateSourceleverages TypeScript's type checking and provides better IDE support.
87-87: Thethrow options.onError(...)pattern is intentional design.The
onErrorfunction is designed to accept athrowsboolean parameter: whenthrows=true(default), it throws internally; whenthrows=false, it returns the error object for the caller to throw. This pattern allows pluggable error handling through the options callback while maintaining control flow flexibility. The implementation is correct as-is.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/provider.component/src/ComponentProvider.ts (1)
55-68: Consider usingNode.ELEMENT_NODEfor consistency.Line 56 uses
node.ELEMENT_NODEwhile the rest of this PR (including other methods in this file) useNode.ELEMENT_NODE. Both are equivalent, but using the globalNodeconstant improves consistency across the codebase.🔎 Proposed consistency fix
- if (node.nodeType !== node.ELEMENT_NODE) { + if (node.nodeType !== Node.ELEMENT_NODE) { return }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/provider.attr/src/AttributeProvider.ts(2 hunks)packages/provider.bindingstring/spec/BindingStringProviderBehaviors.ts(1 hunks)packages/provider.component/src/ComponentProvider.ts(3 hunks)packages/provider.databind/src/DataBindProvider.ts(1 hunks)packages/provider.mustache/src/AttributeMustacheProvider.ts(3 hunks)packages/provider.mustache/src/TextMustacheProvider.ts(2 hunks)packages/provider.virtual/src/VirtualProvider.ts(3 hunks)packages/provider/docs/provider.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/provider/docs/provider.md
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/provider.attr/src/AttributeProvider.ts
- packages/provider.mustache/src/AttributeMustacheProvider.ts
- packages/provider.bindingstring/spec/BindingStringProviderBehaviors.ts
- packages/provider.mustache/src/TextMustacheProvider.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/provider.databind/src/DataBindProvider.ts (2)
packages/bind/src/bindingContext.ts (1)
BindingContext(25-53)packages/bind/src/index.ts (1)
BindingContext(11-11)
⏰ 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 (10)
packages/provider.databind/src/DataBindProvider.ts (3)
6-8: LGTM! Improved readability with Node constant.The use of
Node.ELEMENT_NODEinstead of the numeric literal1improves code readability and maintainability. Theoverridekeyword correctly indicates this method overrides the base class implementation.
14-19: LGTM! Explicit return type and Node constant usage.The method signature correctly narrows the return type to
string | null, and the explicitreturn nullon line 18 ensures all code paths return a value, satisfying thenoImplicitReturnscompiler check.
21-26: LGTM! Type signature properly narrowed.The explicit
return falseon line 25 ensures the method always returns aboolean, eliminating the previousundefinedpossibility and satisfying the stricter TypeScript configuration.packages/provider.component/src/ComponentProvider.ts (4)
14-16: LGTM! Consistent with other provider changes.The change aligns with the pattern used in DataBindProvider and improves readability with the
Node.ELEMENT_NODEconstant.
22-40: LGTM! Explicit return path added.The explicit
return undefinedon line 39 ensures all code paths return a value, satisfying thenoImplicitReturnscompiler check. The SLOT transformation logic remains intact.
42-44: LGTM! Override properly declared.The method correctly uses the
overridekeyword and delegates togetComponentNameForNode.
46-53: LGTM! Override keyword added.The
overridekeyword correctly indicates this method overrides the base class implementation. The early return pattern on line 49 is explicit and appropriate.packages/provider.virtual/src/VirtualProvider.ts (3)
16-32: LGTM! Explicit return path added.The explicit
return undefinedon line 31 ensures all code paths return a value, satisfying thenoImplicitReturnscompiler check. The KO tag transformation logic remains intact.
40-45: LGTM! Explicit return type with Node constant.The method correctly uses
Node.COMMENT_NODEand narrows the return type tostring | null. The explicitreturn nullon line 44 satisfies thenoImplicitReturnscompiler check.
47-52: LGTM! Type signature properly narrowed.The explicit
return falseon line 51 ensures the method always returns aboolean, eliminating the previousundefinedpossibility and satisfying the stricter TypeScript configuration.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/binding.template/spec/templatingBehaviors.ts (1)
538-540: Use Node constants for consistency.Line 539 still uses numeric literals
[1, 3, 8]instead of Node constants, which is inconsistent with the firstTestProvider(Line 481) that correctly usesNode.ELEMENT_NODE. Even though this test is skipped, maintaining consistency improves code maintainability.🔎 Proposed fix
- override get FOR_NODE_TYPES() { - return [1, 3, 8] + override get FOR_NODE_TYPES() { + return [Node.ELEMENT_NODE, Node.TEXT_NODE, Node.COMMENT_NODE] }packages/binding.core/spec/textBehaviors.ts (1)
100-109: Consider removing obsolete commented code.While the
overridemodifier is correct, the commented block (lines 101-106) references IE < 9 workarounds. If this code is truly obsolete and won't be uncommented, consider removing it to reduce clutter.🔎 Proposed cleanup
override nodeHasBindings(/* node, bindingContext */) { - /* // IE < 9 can't bind text nodes, as expando properties are not allowed on them. - // This will still prove that the binding provider was not executed on the children of a restricted element. - if (node.nodeType === Node.TEXT_NODE && jasmine.ieVersion < 9) { - node.data = "replaced"; - return false; - } */ - return true }packages/bind/src/applyBindings.ts (1)
522-528: Consider preserving the original error name.The read-only error handling now correctly creates a new Error instance to ensure
extendsucceeds. However, the original error'snameproperty (e.g.,'DOMException') is lost when constructing the new Error. Preserving the error name improves debugging and error categorization.🔎 Suggested enhancement
} catch (e: any) { // Read-only error e.g. a DOMEXception. spec.stack = error.stack - const message = error.message || String(error) error = new Error(message) + if (error.name && error.name !== 'Error') { + error.name = error.name + } extend(error, spec) }Wait, that diff has a logic error. Let me fix:
} catch (e: any) { // Read-only error e.g. a DOMEXception. spec.stack = error.stack const message = error.message || String(error) + const originalName = error.name error = new Error(message) + if (originalName && originalName !== 'Error') { + error.name = originalName + } extend(error, spec) }This preserves the original error type name (e.g.,
'DOMException','TypeError') for better error identification.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/bind/spec/bindingAttributeBehaviors.ts(2 hunks)packages/bind/src/applyBindings.ts(11 hunks)packages/binding.core/spec/textBehaviors.ts(1 hunks)packages/binding.template/spec/templatingBehaviors.ts(3 hunks)packages/provider.native/src/NativeProvider.ts(3 hunks)packages/provider.virtual/src/VirtualProvider.ts(3 hunks)packages/provider/src/Provider.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/provider/src/Provider.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-21T09:16:36.575Z
Learnt from: phillipc
Repo: knockout/tko PR: 221
File: packages/bind/spec/bindingAttributeBehaviors.ts:135-138
Timestamp: 2025-12-21T09:16:36.575Z
Learning: In the TKO framework, data-bind expressions may use naked arrow function syntax (zero-parameter arrows written without parentheses), e.g., data-bind='click: => was_clicked(true)' or lambdaLiteral: => null. This is valid TKO syntax, though not standard JavaScript. For code reviews, treat this as an allowed syntax specific to TKO in the analyzed file. Ensure tests cover this pattern, update documentation as needed, and avoid relying on parentheses-based parsing in this context. If similar patterns appear elsewhere, consider expanding the rule with a focused pattern to cover related TS files in the binding attribute behaviors area.
Applied to files:
packages/bind/spec/bindingAttributeBehaviors.ts
🧬 Code graph analysis (5)
packages/binding.template/spec/templatingBehaviors.ts (4)
packages/bind/src/bindingContext.ts (3)
bindingContext(56-64)bindingContext(68-161)BindingContext(25-53)packages/binding.if/src/ConditionalBindingHandler.ts (1)
bindingContext(39-41)packages/binding.if/src/with.ts (1)
bindingContext(27-34)packages/binding.if/src/ifUnless.ts (1)
bindingContext(20-30)
packages/provider.native/src/NativeProvider.ts (2)
packages/bind/src/bindingContext.ts (1)
BindingContext(25-53)packages/bind/src/index.ts (1)
BindingContext(11-11)
packages/provider.virtual/src/VirtualProvider.ts (3)
packages/utils/src/index.ts (1)
virtualElements(33-33)packages/bind/src/bindingContext.ts (1)
BindingContext(25-53)packages/bind/src/index.ts (1)
BindingContext(11-11)
packages/bind/src/applyBindings.ts (1)
packages/bind/src/bindingContext.ts (2)
bindingContext(56-64)bindingContext(68-161)
packages/binding.core/spec/textBehaviors.ts (3)
packages/binding.if/src/ConditionalBindingHandler.ts (1)
bindingContext(39-41)packages/binding.if/src/with.ts (1)
bindingContext(27-34)packages/binding.if/src/ifUnless.ts (1)
bindingContext(20-30)
⏰ 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 (17)
packages/bind/spec/bindingAttributeBehaviors.ts (2)
612-612: LGTM! Excellent use of DOM constant.The replacement of the magic number
3withNode.TEXT_NODEimproves code readability and aligns with best practices.
829-843: LGTM! Proper TypeScript strictness applied.The TestProvider class changes correctly implement the stricter TypeScript configuration:
overridemodifiers ensure type safety by confirming these methods exist in the parentProviderclass- Consistent replacement of magic number
3withNode.TEXT_NODEthroughout all methods- Changes align perfectly with the PR objectives for
noImplicitOverride: truepackages/provider.native/src/NativeProvider.ts (5)
16-18: LGTM! Improved readability with Node constants.The
overridemodifier correctly indicates this getter overrides the base class, and usingNode.ELEMENT_NODEandNode.TEXT_NODEinstead of numeric literals improves code clarity.
19-21: LGTM!The
overridemodifier correctly indicates this getter overrides the base class implementation.
23-28: LGTM!The
overridemodifier correctly indicates this method overrides the base class, and the implementation logic is sound.
34-36: LGTM!The
overridemodifier correctly indicates this method overrides the base class. The logic appropriately prevents preprocessor re-entrance when native bindings are present.
52-58: LGTM!The
overridemodifier correctly indicates this method overrides the base class. The implementation properly extracts and transforms native bindings into the expected accessor format.packages/binding.template/spec/templatingBehaviors.ts (2)
479-498: LGTM! Proper use of override modifiers and Node constants.The
overridemodifiers are correctly applied, and the use ofNode.ELEMENT_NODEinstead of the numeric literal1aligns with the PR's objectives to improve type safety and code clarity.
1319-1326: LGTM! Correct use of override modifier.The
overridemodifier is properly applied to thepreprocessNodemethod, and the implementation matches the documented behavior.packages/provider.virtual/src/VirtualProvider.ts (4)
8-10: LGTM! Past review comment addressed.The getter now uses Node constants instead of numeric literals, which improves readability and consistency with other providers. The
overridekeyword properly satisfies thenoImplicitOverrideTypeScript requirement.
16-32: LGTM! TypeScript strictness improvements applied correctly.The
overridekeyword and explicitreturn undefinedstatement properly satisfy the stricter TypeScript compiler checks (noImplicitOverrideandnoImplicitReturns). The explicit return type clarifies the method contract.
40-45: LGTM! Stricter typing and explicit returns implemented correctly.The method now uses the
Node.COMMENT_NODEconstant for better readability, includes theoverridekeyword, and has an explicitreturn nullstatement. These changes properly address the TypeScript compiler strictness requirements.
47-52: LGTM! Return type tightened and explicit returns added.The method now consistently returns
booleaninstead ofboolean | undefined, uses theNode.COMMENT_NODEconstant, and includes explicitreturn false. These changes improve type safety and satisfy the stricter TypeScript requirements.packages/binding.core/spec/textBehaviors.ts (2)
97-98: LGTM! Override modifier and constant usage are correct.The
overridekeyword andNode.ELEMENT_NODEconstant both improve type safety and readability.
110-121: LGTM! Override modifier and node type constant usage are correct.The
overridekeyword andNode.TEXT_NODEconstant improve type safety and readability. The logic correctly prevents binding on generated text nodes, which is important for security.packages/bind/src/applyBindings.ts (2)
35-35: LGTM! Error type tightening with proper handling.The change from
anytoErrorforerrorCapturedimproves type safety throughout the binding error handling flow. The catch block at lines 387-388 correctly wraps non-Error values in Error instances before passing toreportBindingError, ensuring type consistency.Also applies to: 331-331, 387-388
65-65: LGTM! Excellent use of Node type constants.Replacing numeric literals (
1,8) with named constants (Node.ELEMENT_NODE,Node.COMMENT_NODE, etc.) significantly improves code readability and maintainability.Also applies to: 146-146, 344-344, 446-446, 476-476, 496-496
|
@coderabbitai resolve |
✅ Actions performedComments resolved. |
1 participant
Configure Typescript:
Fix some type errors:
Summary by CodeRabbit
Refactor
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.