TS) Enable "strictBindCallApply" #225
Conversation
ts) fixing result type of preprocessNode api) required api change at TextMustacheProvider, see textInterpolationSpec
WalkthroughAPIs and internal providers were widened from Element to Node with runtime Element guards, preprocessors standardized to return null (not undefined) for "no replacement", several method signatures and type annotations tightened, and related tests updated to match the new return/guard semantics. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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 (3)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-12-21T09:16:36.575ZApplied to files:
🧬 Code graph analysis (1)packages/binding.template/spec/nativeTemplateEngineBehaviors.ts (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)
🔇 Additional comments (7)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/provider/src/Provider.ts (1)
131-131: Fix type inconsistency:nodeHasBindingsparameter should beNode.The
LegacyProvider.nodeHasBindingsoverride declaresnode: Element, but the base class method signature at line 41 usesnode: Node. This creates a type incompatibility.🔎 Proposed fix
- override nodeHasBindings(node: Element): boolean { + override nodeHasBindings(node: Node): boolean { return this.providerObject.nodeHasBindings(node) }
🧹 Nitpick comments (2)
packages/computed/src/computed.ts (1)
144-144: Consider using.call()instead of.apply()withargumentscast.Since
writeFunctionexpects exactly one argument (per theComputedWriteFunctionsignature on Line 69), using.call()witharguments[0]would eliminate the need for theanycast while maintaining type safety.🔎 Proposed refactor
- writeFunction.apply(state.evaluatorFunctionTarget, arguments as any) + writeFunction.call(state.evaluatorFunctionTarget, arguments[0] as T)packages/observable/src/Subscription.ts (1)
8-8: LGTM! Type annotation correctly supportsstrictBindCallApply.The explicit type annotation
((node: Node) => void) | nullproperly types the callback used with.bind()on line 29, which is necessary for thestrictBindCallApplycompiler option. The type correctly allows the callback to accept aNodeparameter (even thoughdispose()doesn't use it), and remains consistent with the null initialization and truthiness check.Optional: For consistency, consider adding explicit type annotations to the other private fields (lines 4-7) and constructor parameters (line 11) in a follow-up to fully embrace strict typing.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
packages/bind/spec/nodePreprocessingBehaviors.tspackages/bind/src/applyBindings.tspackages/bind/src/arrayToDomNodeChildren.tspackages/binding.template/spec/foreachBehaviors.tspackages/binding.template/spec/nativeTemplateEngineBehaviors.tspackages/computed/src/computed.tspackages/observable/src/Subscription.tspackages/provider.component/src/ComponentProvider.tspackages/provider.mustache/spec/textInterpolationSpec.tspackages/provider.mustache/src/TextMustacheProvider.tspackages/provider.native/spec/NativeProviderBehaviors.tspackages/provider.native/src/NativeProvider.tspackages/provider.virtual/src/VirtualProvider.tspackages/provider/src/Provider.tspackages/utils/spec/utilsBehaviors.tspackages/utils/src/bind-shim.tstsconfig.json
🧰 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 support "naked" arrow function syntax where zero-parameter arrow functions can be written without parentheses, e.g., `data-bind='click: => was_clicked(true)'` or `lambdaLiteral: => null`. This is valid TKO syntax even though it's not standard JavaScript.
Applied to files:
packages/binding.template/spec/nativeTemplateEngineBehaviors.ts
🧬 Code graph analysis (3)
packages/bind/spec/nodePreprocessingBehaviors.ts (1)
tko.io/src/tko-io.js (1)
node(65-69)
packages/provider/src/Provider.ts (1)
packages/bind/src/bindingContext.ts (1)
BindingContext(25-53)
packages/binding.template/spec/nativeTemplateEngineBehaviors.ts (1)
packages/observable/src/observableArray.ts (2)
ObservableArray(137-141)observableArray(143-157)
🔇 Additional comments (20)
tsconfig.json (1)
23-23: LGTM! Enabling strictBindCallApply strengthens type safety.This compiler option ensures that
.bind(),.call(), and.apply()methods preserve correct parameter types, catching type mismatches at compile time. The accompanying code changes across the codebase properly address the stricter requirements.packages/utils/src/bind-shim.ts (1)
15-15: LGTM! Type cast required for arguments object compatibility.The
as anycast is appropriate here because theargumentsobject is array-like but not a true array. WithstrictBindCallApplyenabled, TypeScript requires explicit casting when usingpush.applywitharguments.packages/provider.mustache/src/TextMustacheProvider.ts (1)
29-34: LGTM! Consistent null return for no-op cases.The explicit return type
Node[] | nulland the null sentinel for "no work done" align with the broader API standardization across preprocessing methods.packages/utils/spec/utilsBehaviors.ts (2)
350-353: LGTM! Modernized to use rest parameters.Replacing the deprecated
argumentsobject with rest parameters (...args) is a cleaner approach and aligns with modern JavaScript practices while satisfying TypeScript's stricter typing requirements.
358-358: LGTM! Type casts appropriate for Function.bind tests.The
as anycasts on bound functions are reasonable for test scenarios that need to bypass TypeScript's strict typing to verify runtime behavior ofFunction.prototype.bind.Also applies to: 371-371, 393-393, 407-407
packages/binding.template/spec/foreachBehaviors.ts (2)
947-948: Excellent addition of runtime type guard!The
instanceof Elementcheck prevents accessingtagNameon non-Element nodes (such as Text or Comment nodes), which would cause runtime errors. This guard is essential for type safety when the preprocessNode signature accepts genericNodetypes.
324-324: LGTM! Consistent null return for no-op preprocessing.Returning
nullinstead ofundefinedstandardizes the "no transformation" sentinel across preprocessing methods, improving API consistency.Also applies to: 962-962
packages/binding.template/spec/nativeTemplateEngineBehaviors.ts (1)
145-161: LGTM! Class-based approach improves type safety.Refactoring from an object literal to a proper TypeScript class with typed properties (
ObservableArray<any>) provides better type checking and aligns with modern TypeScript best practices. The behavior remains functionally equivalent.packages/provider.virtual/src/VirtualProvider.ts (1)
16-32: LGTM! Runtime guard prevents type errors.The
instanceof Elementcheck is essential before accessingtagName, as the parameter type is now the genericNode(which includes Text and Comment nodes). The null return for non-matching cases is consistent with the standardized preprocessing API across the codebase.packages/provider/src/Provider.ts (2)
41-44: LGTM: Broadening API to acceptNodeinstead ofElement.Changing the parameter type from
ElementtoNodemakes these methods more flexible and aligns with the PR's goal of enabling stricter TypeScript settings.
51-53: The return value change contradicts the documented contract, but the reviewer's premise about the previous implementation is incorrect.The current implementation returns
[node], but according to the official documentation (packages/bind/docs/binding-preprocessing.md), the correct pattern is: "If you want to leave 'node' in the DOM, return null or have no 'return' statement." The baseProviderclass should follow this convention—returningnullto indicate "no preprocessing occurred"—not[node].This matters because:
- In
templating.ts, callers checkif (newNodes)before processing the result; returning[node]by default causes unnecessary re-computation- Test cases and subclass implementations (e.g.,
NativeProvider,VirtualProvider) consistently usenullto signal "no-op"- The semantic meaning of
[node]is "I performed preprocessing"; the default should not claim preprocessing occurredHowever, git history shows the previous version also returned
[node], not an implicitundefinedas claimed. The issue is not a recent regression but a longstanding deviation from the documented behavior.Likely an incorrect or invalid review comment.
packages/bind/src/arrayToDomNodeChildren.ts (1)
208-210: LGTM: Improved defensive check.The additional
mapData.mappedNodescheck prevents potential runtime errors whenmapDataexists butmappedNodesis undefined. This is a good defensive programming practice that aligns with the stricter type checking enabled in this PR.packages/provider.native/spec/NativeProviderBehaviors.ts (1)
116-117: LGTM: Test correctly updated to match newpreprocessNodereturn type.The test expectations have been properly updated to reflect that
preprocessNodenow returns an array (Node[]) instead of a single node. The assertions correctly check the array structure and access the first element.packages/provider.mustache/spec/textInterpolationSpec.ts (2)
31-46: LGTM: Test expectations correctly updated fromundefinedtonull.These test updates align with the change in
preprocessNodereturn semantics, wherenullis now used as the "no modification" sentinel instead ofundefined. The changes are consistent across all "no expression" test cases.
87-92: LGTM: Unescaped HTML syntax tests correctly updated.The same
undefinedtonullchange is consistently applied to the unescaped HTML syntax tests, maintaining consistency across the entire spec file.packages/bind/spec/nodePreprocessingBehaviors.ts (2)
39-39: LGTM: Essential runtime guard added for Element-specific properties.The
node instanceof Elementcheck is crucial now thatpreprocessNodeacceptsNodeinstead ofElement. Without this guard, accessingnode.tagNameon a non-Element node (e.g., Text, Comment) would cause a runtime error.
47-47: LGTM: Consistent change fromundefinedtonullfor "no modification" cases.All three test provider implementations now consistently return
nullinstead ofundefinedwhen no preprocessing is needed. This aligns with the updatedpreprocessNodereturn type ofNode[] | null.Also applies to: 80-80, 107-107
packages/provider.component/src/ComponentProvider.ts (2)
22-23: LGTM: Method signature updated with appropriate runtime guard.The signature change to accept
Nodeand returnNode[] | nullis consistent with the baseProviderclass. The runtime guardnode instanceof Elementis essential before accessingtagName, preventing runtime errors when non-Element nodes are passed.
39-39: LGTM: Consistent return ofnullfor "no modification" case.Returning
nullinstead ofundefinedaligns with the updated API semantics across all providers.packages/bind/src/applyBindings.ts (1)
105-105: Type broadening fromChildNodetoNodeis safe and appropriate.The change is consistent with the function signatures that accept
currentChild. The variable is assigned fromnextInQueue(which isChildNode | null) and passed to functions acceptingNodeparameters. NoChildNode-specific properties are accessed oncurrentChild— it is only passed as an argument tovirtualElements.nextSibling(),preprocessNode(), andapplyBindingsToNodeAndDescendantsInternal(), all of which accept the broaderNodetype.
| override preprocessNode(node: Element) { | ||
| return node[NATIVE_BINDINGS] ? node : null | ||
| return node[NATIVE_BINDINGS] ? [node] : null | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -nP --type=ts -C3 'preprocessNode\s*\(' -g '!*.spec.ts' -g '!*Behaviors.ts'Repository: knockout/tko
Length of output: 4952
🏁 Script executed:
rg -nP --type=ts 'preprocessNode' packages/provider.native/src/NativeProvider.ts -B5 -A5Repository: knockout/tko
Length of output: 399
🏁 Script executed:
fd . packages/provider.native/src -name "*.ts" -type f | head -20Repository: knockout/tko
Length of output: 286
🏁 Script executed:
rg -nP --type=ts 'preprocessNode' -B3 -A3 | grep -v 'spec\|Behaviors' | head -100Repository: knockout/tko
Length of output: 7001
🏁 Script executed:
fd . packages -name "*.ts" -type f | xargs rg -l "preprocessNode" | grep -v spec | grep -v BehaviorsRepository: knockout/tko
Length of output: 744
🏁 Script executed:
rg -nP 'preprocessNode' packages/binding.handler/src -B3 -A3Repository: knockout/tko
Length of output: 124
🏁 Script executed:
rg -nP --type=ts 'preprocessNode' packages/bind/src/applyBindings.ts -B5 -A15Repository: knockout/tko
Length of output: 1452
🏁 Script executed:
cat packages/bind/src/applyBindings.ts | head -150Repository: knockout/tko
Length of output: 5223
🏁 Script executed:
cat -n packages/bind/src/applyBindings.ts | sed -n '100,130p'Repository: knockout/tko
Length of output: 1571
🏁 Script executed:
rg -nP --type=ts 'preprocessNode' packages/bind/src/applyBindings.ts -B2 -A10 | head -50Repository: knockout/tko
Length of output: 1318
🏁 Script executed:
cat -n packages/binding.template/src/templating.ts | sed -n '69,100p'Repository: knockout/tko
Length of output: 1712
🏁 Script executed:
cat -n packages/bind/src/applyBindings.ts | sed -n '109,121p'Repository: knockout/tko
Length of output: 868
🏁 Script executed:
cat -n packages/binding.template/src/templating.ts | sed -n '74,86p'Repository: knockout/tko
Length of output: 656
🏁 Script executed:
cat -n packages/provider/src/Provider.ts | sed -n '48,53p'Repository: knockout/tko
Length of output: 226
Critical issue: applyBindings.ts ignores the return value from preprocessNode.
The method signature requires Node[] | null to be returned and handled. While templating.ts correctly captures and uses the array return value (accessing indices like newNodes[0]), applyBindings.ts calls preprocessNode.call(provider, currentChild) at line 116 but discards the return value entirely. This breaks the preprocessing contract and prevents proper node transformation in that code path.
🤖 Prompt for AI Agents
In packages/provider.native/src/NativeProvider.ts around lines 34–36 you
implement preprocessNode to return Node[] | null, but elsewhere
(applyBindings.ts at line 116) the return value is ignored; update the
applyBindings.ts call to capture the result of preprocessNode.call(provider,
currentChild) into a const (e.g., newNodes), check for a non-null array, and
then replace or splice the currentChild with the returned newNodes (or proceed
normally when null) so the Node[] | null contract is honored and transformed
nodes are applied.
brianmhunt
left a comment
brianmhunt
left a comment
There was a problem hiding this comment.
👍 looks wonderful, thank you. We just need to ensure the invocations test either works or is updated.
| } | ||
| } | ||
| if (mapData) { | ||
| if (mapData && mapData.mappedNodes) { |
There was a problem hiding this comment.
can shorten to mapData?.mappedNodes
| it('Can modify the set of top-level nodes in a foreach loop', function () { | ||
| options.bindingProviderInstance.preprocessNode = function (node) { | ||
| // Replace <data /> with <span data-bind="text: $data"></span> | ||
| if (!(node instanceof Element)) return null |
There was a problem hiding this comment.
I prefer to keep return statements bound in braces i.e. { return null } > return null to avoid the accidental newline breaking the condition, dangling else. Not a big issue, just noting my style preference - particularly not an issue here in a spec/ file :)
| ]) | ||
| } | ||
|
|
||
| invocationCount = function () { |
There was a problem hiding this comment.
the this inside function will be the this of the anonymous function (), solved with a method or lambda e.g.
invocationCount () { ++this.invocations }
// or
invocationCount = () => ++this.invocationsI would expect that invocations will be 0 no matter what here. It's not clear from the code review whether invocations being > 0 is validated.
There was a problem hiding this comment.
@CodeRabbit please review this comment ^^ and comment on the context and the impact
There was a problem hiding this comment.
🧩 Analysis chain
@brianmhunt You've identified a real concern here. Let me break down the context and impact:
Context
This test is verifying that nested templates in Knockout don't re-render unnecessarily. The invocationCount function is called from within a data-bind expression:
<span data-bind='text: $root.invocationCount()'></span>The test expects invocations to increment correctly (1, 2, 3, 4, 5, 6...) as shown in the assertions.
The Issue
Using function() syntax creates its own this binding context. When Knockout evaluates $root.invocationCount(), the this inside the function will depend on how Knockout invokes it:
- If called as
viewModel.invocationCount(),thisshould beviewModel✓ - If the function reference is extracted and called without context,
thiswould beundefined(strict mode) orwindow✗
While Knockout typically preserves the object context when calling methods via bindings, this approach is fragile and depends on internal framework behavior.
Impact
- If the test passes: The code works but relies on Knockout's implementation details
- If the test fails: This
thisbinding issue is definitely the cause, and invocations will fail to increment
Recommendation
Use one of these safer alternatives:
// Arrow function (captures `this` from class scope)
invocationCount = () => ++this.invocations
// or class method
invocationCount() {
return ++this.invocations
}Both ensure this always refers to the class instance regardless of how the function is called.
🏁 Script executed:
#!/bin/bash
# Check if there are test results or CI status for this PR
gh pr view 225 --json statusCheckRollup --jq '.statusCheckRollup[] | select(.name | contains("test") or contains("Test") or contains("CI")) | {name: .name, status: .status, conclusion: .conclusion}'
Length of output: 202
Update: Test Status
Good news - the tests are passing ✅ (both testheadless runs succeeded), which confirms that invocations is incrementing correctly and Knockout is preserving the this context when evaluating $root.invocationCount() from the binding.
However, Brian's Concern is Still Valid
While the code works, using function() syntax here is:
- Less readable - Class methods/arrow functions clearly signal that
thisrefers to the class instance - More fragile - Relies on Knockout's internal binding evaluation behavior
- Not idiomatic - Modern TypeScript classes typically use methods or arrow functions for class members
Recommendation
Even though tests pass, I'd recommend changing to one of these more robust patterns:
// Arrow function (clearest for this use case)
invocationCount = () => ++this.invocations
// or class method
invocationCount() {
return ++this.invocations
}This would make the code more maintainable and explicit about the intended this binding, without relying on framework implementation details.
🧬 Code Graph Analysis Results
packages/binding.template/spec/nativeTemplateEngineBehaviors.ts
[relevant_code_snippets]
-
File: packages/binding.template/src/templateEngine.ts
Lines 101-109:renderTemplate( template: string | Node, bindingContext: BindingContext<any>, options: TemplateOptions<any>, templateDocument?: Document ): Node[] { const templateSource = this.makeTemplateSource(template, templateDocument) return this.renderTemplateSource(templateSource, bindingContext, options, templateDocument) } -
File: packages/binding.template/src/templateSources.ts
Lines 143-163:export class anonymousTemplate extends domElement { constructor(element: Element | Comment) { super(element) } override text(): string override text(valueToWrite: string): undefined 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 } } -
File: packages/binding.template/src/templating.ts
Lines 186-244:export function renderTemplate<T = any>( template: string | Node | (() => string | Node), dataOrBindingContext: T | BindingContext<T> | null | undefined, options: TemplateOptions<T> | null | undefined, targetNodeOrNodeArray: Node | Node[], renderMode?: RenderModeEnum, afterBindingCallback? ): Computed<void> | string { options = options || {} if ((options.templateEngine || _templateEngine) === undefined) { throw new Error('Set a template engine before calling renderTemplate') } renderMode = renderMode || 'replaceChildren' if (targetNodeOrNodeArray) { let firstTargetNode = getFirstNodeFromPossibleArray(targetNodeOrNodeArray) const whenToDispose = function () { return !firstTargetNode || !domNodeIsAttachedToDocument(firstTargetNode) } // Passive disposal (on next evaluation) const activelyDisposeWhenNodeIsRemoved = firstTargetNode && renderMode === 'replaceNode' ? firstTargetNode.parentNode : firstTargetNode return computed( // So the DOM is automatically updated when any dependency changes function () { // Ensure we've got a proper binding context to work with const bindingContext = dataOrBindingContext && dataOrBindingContext instanceof BindingContextConstructor ? dataOrBindingContext : new BindingContextConstructor(dataOrBindingContext, undefined, undefined, undefined, { exportDependencies: true }) const templateName = resolveTemplateName(template, bindingContext.$data, bindingContext) const renderedNodesArray = executeTemplate( targetNodeOrNodeArray, renderMode, templateName, bindingContext, options, afterBindingCallback ) if (renderMode === 'replaceNode') { targetNodeOrNodeArray = renderedNodesArray firstTargetNode = getFirstNodeFromPossibleArray(targetNodeOrNodeArray) } }, null, { disposeWhen: whenToDispose, disposeWhenNodeIsRemoved: activelyDisposeWhenNodeIsRemoved } ) } else { // We don't yet have a DOM node to evaluate, so use a memo and render the template later when there is a DOM node return memoization.memoize(function (domNode) { renderTemplate(template, dataOrBindingContext, options, domNode, 'replaceNode') }) } } -
File: packages/provider.databind/src/DataBindProvider.ts
Lines 5-27:export default class DataBindProvider extends BindingStringProvider { override get FOR_NODE_TYPES() { return [Node.ELEMENT_NODE] } get BIND_ATTRIBUTE() { return 'data-bind' } override getBindingString(node: Node): string | null { if (node.nodeType === Node.ELEMENT_NODE) { return (node as Element).getAttribute(this.BIND_ATTRIBUTE) } return null } override nodeHasBindings(node: HTMLElement, context?: BindingContext): boolean { if (node.nodeType === Node.ELEMENT_NODE) { return node.hasAttribute(this.BIND_ATTRIBUTE) } return false } } -
File: packages/bind/src/applyBindings.ts
Lines 483-502:export function applyBindings<T = any>( viewModelOrBindingContext: BindingContext<T> | Observable<T> | T, rootNode: HTMLElement, extendContextCallback?: BindingContextExtendCallback<T> ): Promise<unknown> { const asyncBindingsApplied = new Set() // rootNode is optional if (!rootNode) { rootNode = window.document.body if (!rootNode) { throw Error('ko.applyBindings: could not find window.document.body; has the document been loaded?') } } else if (rootNode.nodeType !== Node.ELEMENT_NODE && rootNode.nodeType !== Node.COMMENT_NODE) { throw Error('ko.applyBindings: first parameter should be your view model; second parameter should be a DOM node') } const rootContext = getBindingContext<T>(viewModelOrBindingContext, extendContextCallback) applyBindingsToNodeAndDescendantsInternal(rootContext, rootNode, asyncBindingsApplied) return Promise.all(asyncBindingsApplied) } -
File: packages/observable/src/observable.ts
Lines 87-121:export function observable<T = any>(initialValue?: T): Observable { function Observable() { if (arguments.length > 0) { // Write // Ignore writes if the value hasn't changed // inherits from interface SubscribableFunctions if ((Observable as any).isDifferent(Observable[LATEST_VALUE], arguments[0])) { ;(Observable as any).valueWillMutate() Observable[LATEST_VALUE] = arguments[0] ;(Observable as any).valueHasMutated() } return this // Permits chained assignments } else { // Read dependencyDetection.registerDependency(Observable) // The caller only needs to be notified of changes if they did a "read" operation return Observable[LATEST_VALUE] } } overwriteLengthPropertyIfSupported(Observable as any, { value: undefined }) Observable[LATEST_VALUE] = initialValue subscribable.fn.init(Observable) // Inherit from 'observable' Object.setPrototypeOf(Observable, observable.fn) if (options.deferUpdates) { deferUpdates(Observable) } // through setPrototypeOf we can cast to Observable return Observable as unknown as Observable }
There was a problem hiding this comment.
fixed -> invocationCount() { return ++this.invocations }
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| override preprocessNode(node: Element): Node[] | undefined { | ||
| if (node.tagName === 'KO') { | ||
| override preprocessNode(node: Node): Node[] | null { | ||
| if (node instanceof Element && node.tagName === 'KO') { |
There was a problem hiding this comment.
this could be a duck-type e.g. if ('tagName' in node && node.tagName === 'KO'), which is more generic - but practically speaking probably doesn't matter here.
There was a problem hiding this comment.
"Instanceof" has the advantage that the TypeScript type tracking is corrected. From here on, ts understands that it is an element.
| if (!Function.prototype['bind']) { | ||
| // Shim/polyfill JavaScript Function.bind. | ||
| // This implementation is based on the one in prototype.js | ||
| // Old browsers (IE7 and earlier) do not implement Function.bind. |
There was a problem hiding this comment.
practically speaking we've got bigger issues for folks on IE7 lol
There was a problem hiding this comment.
I'm looking forward to the moment when we can delete all of this. Currently, I'm tagging things like this so I can search for them using IE.
|
Coderabbit drags me down the rabbit hole. The PR is getting bigger. I have also fixed the provider typings. |
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 (2)
packages/provider.multi/src/MultiProvider.ts (2)
51-58: Add explicit return type and returnnullinstead ofundefined.According to the PR objectives,
preprocessNodeshould returnNode[] | null, but the implementation implicitly returnsundefinedwhen no provider returnsnewNodes. Additionally, the method lacks an explicit return type annotation.🔎 Proposed fix
-override preprocessNode(node: Node) { +override preprocessNode(node: Node): Node[] | null { for (const provider of this.providersFor(node)) { const newNodes = provider.preprocessNode(node) if (newNodes) { return newNodes } } + return null }
60-71: Add type annotation for thecontextparameter.The
contextparameter is missing a type annotation. It should beBindingContextto match the type used ingetBindingAccessorsand improve type safety.🔎 Proposed fix
-*enumerateProviderBindings(node: Node, context) { +*enumerateProviderBindings(node: Node, context: BindingContext) { for (const provider of this.providersFor(node)) {
🧹 Nitpick comments (2)
packages/provider.multi/src/MultiProvider.ts (1)
73-82: Consider adding an explicit return type annotation.While TypeScript can infer the return type, adding an explicit annotation improves readability and ensures the method signature is clear, especially for an overridden method.
💡 Suggested enhancement
-override getBindingAccessors(node: Node, context?: BindingContext) { +override getBindingAccessors(node: Node, context?: BindingContext): Record<string, any> { const bindings = {}Note: The exact return type should match the base
Providerclass signature.packages/provider.component/src/ComponentProvider.ts (1)
55-58: Redundant condition: instanceof Element implies nodeType === ELEMENT_NODE.Line 56-57 checks both
!(node instanceof Element)andnode.nodeType !== Node.ELEMENT_NODE. The second check is redundant because Element instances always havenodeType === Node.ELEMENT_NODE.🔎 Simplify to single instanceof check
- getComponentNameForNode(node: Node): string | null{ + getComponentNameForNode(node: Node): string | null { if (!(node instanceof Element)) { - return null - } - if (node.nodeType !== Node.ELEMENT_NODE) { return null }Note: This also fixes the extra space in the return type annotation.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/binding.template/spec/templatingBehaviors.tspackages/provider.attr/src/AttributeProvider.tspackages/provider.bindingstring/src/BindingStringProvider.tspackages/provider.component/src/ComponentProvider.tspackages/provider.multi/src/MultiProvider.tspackages/provider.mustache/src/AttributeMustacheProvider.tspackages/provider.native/src/NativeProvider.tspackages/provider.virtual/src/VirtualProvider.tspackages/provider/src/Provider.ts
🧰 Additional context used
🧬 Code graph analysis (4)
packages/provider.mustache/src/AttributeMustacheProvider.ts (1)
packages/bind/spec/bindingAttributeBehaviors.ts (2)
TestProvider(828-853)node(942-946)
packages/provider.component/src/ComponentProvider.ts (3)
packages/bind/spec/nodePreprocessingBehaviors.ts (6)
testNode(15-138)TestProvider(60-82)TestProvider(59-94)TestProvider(96-137)TestProvider(97-109)preprocessNode(61-81)packages/binding.component/spec/componentBindingBehaviors.ts (3)
testNode(1284-1318)testNode(1206-1229)testNode(746-765)packages/bind/spec/bindingAttributeBehaviors.ts (1)
TestProvider(828-853)
packages/provider.multi/src/MultiProvider.ts (4)
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/bind/src/bindingContext.ts (1)
BindingContext(25-53)
packages/provider.attr/src/AttributeProvider.ts (2)
packages/binding.core/src/attr.ts (1)
attr(7-46)packages/utils.parser/src/Parser.ts (1)
name(115-144)
🪛 GitHub Actions: Check code formatting with Prettier
packages/provider.component/src/ComponentProvider.ts
[error] 1-1: Prettier formatting check failed. Run 'npx prettier --write' to fix code style issues in this file. (Triggered by 'npx prettier . --check')
🔇 Additional comments (16)
packages/provider.multi/src/MultiProvider.ts (3)
8-8: LGTM: Appropriate type tightening.The change from
any[]tonumber[]correctly reflects that node types are numeric constants, improving type safety.
43-45: LGTM: Safe API broadening from Element to Node.The method only accesses
node.nodeType, which is available on all Node objects, making this broadening safe.
47-49: LGTM: Correct return type tightening.The explicit
booleanreturn type correctly reflects thatArray.some()always returns a boolean, tightening from the previousboolean | undefined.packages/provider.bindingstring/src/BindingStringProvider.ts (1)
56-58: LGTM: Return type narrowed to exclude undefined.The return type change from
string | null | undefinedtostring | nullis a reasonable API tightening that aligns with the PR's goal of stricter typing. The falsy check at line 48 (if (!bindingString)) handles both null and undefined correctly.packages/provider.mustache/src/AttributeMustacheProvider.ts (2)
45-50: Defensive runtime guards added for Node-to-Element narrowing.The instanceof Element check correctly prevents attribute access on non-Element nodes. This guard aligns with FOR_NODE_TYPES declaration at line 25 limiting to ELEMENT_NODE, providing defense-in-depth for type safety.
98-101: Consistent guard pattern for non-Element nodes.The guard matches the pattern in nodeHasBindings (lines 46-48), correctly returning false for non-Element nodes before attempting Element-specific operations.
packages/binding.template/spec/templatingBehaviors.ts (1)
542-554: Test provider correctly updated to Node-based API.The test provider implementation mirrors the production pattern: accepting Node parameters with instanceof Element guards. The guard in getBindingAccessors returns an empty object (line 548) rather than false, which is appropriate for binding accessors.
packages/provider.attr/src/AttributeProvider.ts (2)
18-27: Defensive guard prevents attribute access on non-Element nodes.The instanceof Element check (lines 19-21) correctly returns an empty array for non-Element nodes, preventing errors when accessing
node.attributeson incompatible node types.
39-39: Good: Replaced deprecatedsubstrwithsubstring.The change from
attr.name.substr(this.PREFIX.length)tosubstring(this.PREFIX.length)modernizes the code, assubstris deprecated.packages/provider/src/Provider.ts (3)
17-17: LGTM: Explicit type alias improves clarity.The BindingAccessors type alias clearly documents the expected shape of binding accessor objects, improving code readability and type safety.
139-144: API change: LegacyProvider now returns null instead of undefined.The explicit
return nullat line 143 (when no preprocessor exists) changes the sentinel value from implicit undefined to explicit null. This aligns with the broader API standardization but is technically a breaking change for code that checks=== undefined.
55-57: Return type is properly handled by callers.The
preprocessNodemethod signature withNode[] | nullis well-supported throughout the codebase. The base implementation returns[node]and never null. Key internal callerMultiProvider.preprocessNode()(line 53-55) explicitly checksif (newNodes)before using the result, properly handling both array and null returns. Multiple subclass overrides (VirtualProvider,NativeProvider,ComponentProvider) already declare theNode[] | nullreturn type and are invoked correctly. Tests verify the return type is handled as expected.packages/provider.virtual/src/VirtualProvider.ts (1)
16-32: Type-safe Element check preferred over duck-typing.The
instanceof Elementcheck at line 17 is more robust than the previously suggested duck-type check ('tagName' in node). The instanceof approach provides proper type narrowing for TypeScript and prevents edge cases where non-Element nodes might have a tagName property.Note: This addresses the past review comment suggesting duck-typing. The instanceof approach is preferable for type safety.
packages/provider.component/src/ComponentProvider.ts (3)
22-40: LGTM: SLOT preprocessing with proper guards.The instanceof Element check at line 23 correctly limits SLOT transformation to Element nodes, and the explicit
return nullat line 39 aligns with the new API contract.
46-53: Good: Object.create(null) instead of undefined for empty accessors.Returning
Object.create(null)at line 49 for components without bindings is more explicit than undefined and provides a proper empty object without prototype pollution concerns.
70-74: Defensive default for non-Element params.The guard returns a safe default
{ $raw: {} }for non-Element nodes, preventing errors in downstream code that expects the $raw property.
| override preprocessNode(node: Node): Node[] | null { | ||
| return node[NATIVE_BINDINGS] ? [node] : null | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the preprocessNode call in applyBindings.ts around line 116
rg -nP --type=ts 'preprocessNode' packages/bind/src/applyBindings.ts -B5 -A10Repository: knockout/tko
Length of output: 168
🏁 Script executed:
# Find the preprocessNode interface/contract definition
rg -nP --type=ts 'preprocessNode\s*\(' packages/ -B2 -A2 --max-count=20Repository: knockout/tko
Length of output: 11943
🏁 Script executed:
# Check the full context of how preprocessNode should be used
fd -e ts -path "*/src/*" | xargs grep -l "preprocessNode" | head -10Repository: knockout/tko
Length of output: 224
🏁 Script executed:
# Find applyBindings.ts file
fd -name "applyBindings.ts" packages/Repository: knockout/tko
Length of output: 286
🏁 Script executed:
# Search for applyBindings file location more broadly
rg -l "applyBindings" packages/ --type=ts | grep -i applyRepository: knockout/tko
Length of output: 90
🏁 Script executed:
# Read applyBindings.ts with line numbers, focusing on the area around line 116
sed -n '100,150p' packages/bind/src/applyBindings.ts | cat -nRepository: knockout/tko
Length of output: 2386
🏁 Script executed:
# Get the full file to understand context
wc -l packages/bind/src/applyBindings.tsRepository: knockout/tko
Length of output: 94
🏁 Script executed:
# Get more context around line 116 in applyBindings.ts
sed -n '110,130p' packages/bind/src/applyBindings.ts | cat -nRepository: knockout/tko
Length of output: 1226
Critical: applyBindings.ts ignores preprocessNode return value.
At line 116 in packages/bind/src/applyBindings.ts, the code calls preprocessNode.call(provider, currentChild) but discards the return value. Since preprocessNode returns Node[] | null to indicate whether the node should be replaced with preprocessed nodes, ignoring this return value breaks the preprocessing contract entirely. Providers that attempt to transform nodes (as shown in MultiProvider's correct pattern at lines 51–55) will have no effect.
Capture the return value and replace currentChild with the returned nodes when non-null, or skip preprocessing for that node when null is returned.
There was a problem hiding this comment.
This section in applybindings.ts was not been modified. The return value was ignored here before as well.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for all usages of preprocessNode to understand the pattern
rg -nP --type=ts 'preprocessNode' packages/bind/src/ -B3 -A5
Length of output: 1546
🏁 Script executed:
#!/bin/bash
# Check MultiProvider's handling of preprocessNode return value mentioned in the review
rg -nP --type=ts 'preprocessNode' packages/provider.multi/src/MultiProvider.ts -B5 -A10
Length of output: 613
🏁 Script executed:
#!/bin/bash
# Look for the full context around line 116 in applyBindings.ts
sed -n '100,140p' packages/bind/src/applyBindings.ts | cat -n
Length of output: 1919
@phillipc You're absolutely right - I apologize for the confusion. Looking at the code, line 116 in applyBindings.ts was not modified in this PR, and the return value from preprocessNode was already being ignored in the existing implementation.
This PR only changed the type signatures to be more explicit (returning Node[] | null instead of the previous types), but didn't introduce the behavior of ignoring the return value. That's pre-existing code.
Since this is outside the scope of your PR (which focuses on enabling strictBindCallApply), it doesn't need to be addressed here. Thank you for the clarification!
🧬 Code Graph Analysis Results
[packages/provider/src/Provider.ts:19-99]
export default class Provider {
constructor(params?: ProviderParamsInput | null) {
if (this.constructor === Provider) {
throw new Error('Provider is an abstract base class.')
}
if (!('FOR_NODE_TYPES' in this)) {
// FOR_NODE_TYPES must return a list of integers corresponding to the
// node.nodeType's that the provider handles.
throw new Error('Providers must have FOR_NODE_TYPES property')
}
this.bindingHandlers = params?.bindingHandlers || new BindingHandlerObject()
this.globals = params?.globals || {}
}
get FOR_NODE_TYPES(): number[] {
throw new Error('Providers must override FOR_NODE_TYPES property')
}
setGlobals(globals) {
this.globals = globals
}
get preemptive() {
return false
}
nodeHasBindings(node: Node, context?: BindingContext): boolean {
return false
}
getBindingAccessors(node: Node, context?: BindingContext): BindingAccessors {
return Object.create(null)
}
/**
- Preprocess a given node.
- @param {Node} node
- @returns {[Node]|null}
/
preprocessNode(node: Node): Node[] | null {
return [node]
}
postProcess(/ node */) {}
bindingHandlers: BindingHandlerObject
globals: any | undefined
_overloadInstance: any | undefined
/** For legacy binding provider assignments to
- ko.bindingProvider.instance = ... */
get instance() {
return this._overloadInstance || this
}
set instance(provider) {
if (!provider || provider === this) {
this._overloadInstance = undefined
} else {
this._overloadInstance = new LegacyProvider(provider, this)
}
}
// Given a function that returns bindings, create and return a new object that contains
// binding value-accessors functions. Each accessor function calls the original function
// so that it always gets the latest value and all dependencies are captured. This is used
// by ko.applyBindingsToNode and getBindingsAndMakeAccessors.
makeAccessorsFromFunction(callback) {
return objectMap(dependencyDetection.ignore(callback), (value, key) => () => callback()[key])
}
// Returns the valueAccessor function for a binding value
makeValueAccessor(value): Function {
return () => value
}
// Given a bindings function or object, create and return a new object that contains
// binding value-accessors functions. This is used by ko.applyBindingsToNode.
makeBindingAccessors(bindings, context, node) {
if (typeof bindings === 'function') {
return this.makeAccessorsFromFunction(bindings.bind(null, context, node))
} else {
return objectMap(bindings, this.makeValueAccessor)
}
}
}
[packages/bind/src/bindingContext.ts:25-53]
export interface BindingContext<T = any> {
ko: KnockoutInstance
$parent?: any
$parents: any[]
$root: any
$data: T
$rawData: T | Observable
$index?: Observable
$parentContext?: BindingContext
// $componentTemplateNodes: any; added in makeChildBindingContext to context
// $componentTemplateSlotNodes; added in makeChildBindingContext to context
$component?: any
extend(properties: object): BindingContext
extend(properties: (self: BindingContext) => object): BindingContext
lookup(token: string, globals: any, node: any)
createChildContext(
dataItemOrAccessor: any,
dataItemAlias?: string,
extendCallback?: Function,
settings?: BindingContextSetting
): BindingContext
createStaticChildContext(dataItemOrAccessor: any, dataItemAlias: any): BindingContext
}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/provider.component/src/ComponentProvider.ts (1)
18-40: Update JSDoc to reflect the new parameter type.The JSDoc comment on line 20 still references
@param {Element} node, but the method signature now acceptsNode. The implementation correctly guards Element-specific access withinstanceof Elementon line 23.🔎 Proposed fix
/** * Convert <slot name='X'> to <!-- ko slot: 'X' --><!-- /ko --> - * @param {Element} node + * @param {Node} node */
🧹 Nitpick comments (1)
packages/provider.component/src/ComponentProvider.ts (1)
55-68: Consider simplifying the redundant type guard.The condition on line 56 checks both
!(node instanceof Element)andnode.nodeType !== Node.ELEMENT_NODE. However, ifnode instanceof Elementis true, thennode.nodeTypewill always beNode.ELEMENT_NODE, making the second check redundant.🔎 Proposed simplification
getComponentNameForNode(node: Node): string | null { - if (!(node instanceof Element) || node.nodeType !== Node.ELEMENT_NODE) { + if (!(node instanceof Element)) { return null }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/bind/src/arrayToDomNodeChildren.tspackages/provider.component/src/ComponentProvider.tspackages/provider.multi/src/MultiProvider.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/provider.multi/src/MultiProvider.ts
🔇 Additional comments (4)
packages/bind/src/arrayToDomNodeChildren.ts (1)
208-210: LGTM! Optional chaining correctly implemented.The use of optional chaining
mapData?.mappedNodesproperly handles the case wheremapDatamay benull(set on line 203) and makes the code more concise and idiomatic.packages/provider.component/src/ComponentProvider.ts (3)
42-44: LGTM!The method correctly delegates to
getComponentNameForNodeand handles the Node parameter type appropriately.
46-53: LGTM!The migration from Element to Node parameter type is well-handled, and the change from returning
undefinedtoObject.create(null)aligns with the broader API standardization mentioned in the PR objectives.
70-84: LGTM!The defensive guard correctly prevents Element-specific operations on non-Element nodes, and the early return with a consistent object shape (
{ $raw: {} }) maintains API predictability.
@coderabbitai: These are relevant topics in the current TKO codebase. Please find more and create a new PR to correct them. 🫡 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/provider.component/src/ComponentProvider.ts (1)
55-68: Consider simplifying the Element guard.The nodeType check on line 56 appears redundant—all Element instances have nodeType === Node.ELEMENT_NODE by definition. The instanceof Element check alone should suffice.
🔎 Simplified guard
- if (!(node instanceof Element) || node.nodeType !== Node.ELEMENT_NODE) { + if (!(node instanceof Element)) { return null }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/provider.component/src/ComponentProvider.tspackages/provider.native/spec/NativeProviderBehaviors.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/provider.native/spec/NativeProviderBehaviors.ts
⏰ 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 (4)
packages/provider.component/src/ComponentProvider.ts (4)
18-40: LGTM! Proper Element guard for SLOT processing.The instanceof Element guard on line 23 correctly ensures that tagName is only accessed on Element nodes. The explicit null return aligns with the PR's goal of standardizing return semantics.
42-44: LGTM! Consistent parameter type widening.The change to accept Node instead of Element is consistent with the broader API surface changes. The Boolean wrapper correctly handles the null return from getComponentNameForNode.
46-53: Good choice using Object.create(null) for empty accessors.Returning Object.create(null) instead of undefined when no component is found is safer and more consistent. This prevents prototype pollution while maintaining the semantic of "no bindings available."
70-84: LGTM! Proper guard and safe default for non-Element nodes.The instanceof Element guard correctly protects Element-specific operations (getAttribute, etc.). Returning { $raw: {} } for non-Element nodes is consistent with the empty params fallback on line 79.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 45853 🏁 Script executed: Length of output: 3744 🏁 Script executed: Length of output: 5449 🏁 Script executed: Length of output: 9083 @phillipc: I've found numerous methods across the provider codebase that are missing explicit return type annotations! This is great cleanup work to do alongside Methods Missing Explicit Return Typespackages/provider.multi/src/MultiProvider.ts
packages/provider/src/Provider.ts (LegacyProvider)
packages/provider.mustache/src/TextMustacheProvider.ts
packages/provider.attr/src/AttributeProvider.ts
packages/provider.bindingstring/src/BindingStringProvider.ts
packages/provider.component/src/ComponentProvider.ts
packages/provider.native/src/NativeProvider.ts
packages/provider.mustache/src/AttributeMustacheProvider.ts
Would you like me to create a GitHub issue to track this work? I can include all these findings with suggested return types for each method. |
|
@brianmhunt: I'm finished with the pull request. If you follow the existing code, then |
brianmhunt
left a comment
brianmhunt
left a comment
There was a problem hiding this comment.
Approving, a couple style comments worth noting / keeping in mind, but nothing blocking. @phillipc please feel free to revise based on the comments (or not, all good) and merge at will. Thank you!
| providers?: any[] | ||
| } | ||
|
|
||
| export type BindingAccessors = { [name: string]: Function } |
There was a problem hiding this comment.
Functionis a broad definition, would be nice to narrow this type if we could. Not a blocker, just noting.
| return node.tagName == 'EM' || originalBindingProvider.nodeHasBindings(node, bindingContext) | ||
| override nodeHasBindings(node: Node, bindingContext?: BindingContext) { | ||
| if (node instanceof Element) | ||
| return node.tagName == 'EM' || originalBindingProvider.nodeHasBindings(node, bindingContext) |
There was a problem hiding this comment.
if (condition) followed by newline is dangerous. Better to always use braces. Not a blocker, but noting.
2 participants
ts) set strictBindCallApply to true
ts) fixing result type of preprocessNode
api) required api change at TextMustacheProvider, see textInterpolationSpec
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.