Typescript) fix errors #202

phillipc · 2025-01-12T17:18:19Z

As announced, we have started to make TKO more type-safe. To get a startpoint, we have set the tsconfig as softly as possible and continue to allow "any" types.

We (@Auge19 , @mcselle ) validate the typescript code with tsc. The current status is only 313 errors left. We began with 7410 errors. With this draft PR we would like to get feedback and start our own review. The goal is 0 errors with tsc. If this PR is ready and merged, we would like to continue with ESLINT.

All tests continue to pass.

add some types

#we should remove jasmine..

switch d.ts-version for jasmine (the project used jasmine 1.3)

1) [] => new Array 2) childNodes => children 3) extend some global type-defs

…_typed

fix wrong jasmine imports fix method signature

…ong_typed

…yped

brianmhunt

👍 Great changes thank you. Will keep checking back and reviewing!

brianmhunt · 2025-03-04T14:09:38Z

builds/knockout/src/index.ts

  options: {
    bindingGlobals: defaultOptions.global,
    bindingStringPreparsers: [functionRewrite]
  }
 })

+// @ts-ignore: Build-Parameter


also declare const

brianmhunt · 2025-03-04T14:13:42Z

packages/bind/spec/bindingDependencyBehaviors.ts

      expect(clickedVM).toEqual(vm());

            // set the view-model to a new object
      vm({ someProp: observableConstructor('My new prop value'), checkVM: checkVM });
-      expect(testNode.childNodes[0].childNodes[0].value).toEqual('My new prop value');
+      expect(child.value).toEqual('My new prop value');


To make sure it has the latest DOM structure, child should be a function i.e.

const child = () => testNode.childNodes[0].childNodes[0] as HTMLInputElement

... because part of the assertion here is the DOM structure i.e. testNode.childNodes[0].childNodes[0] may no longer be child here, and if that were the case we'd want this test to fail. That failure wouldn't be visible if input is a reference to the input.

packages/binding.core/spec/hasfocusBehaviors.ts

packages/provider.mustache/src/mustacheParser.ts

packages/utils/helpers/jasmine-13-helper.ts

packages/utils/src/dom/html.ts

packages/utils/src/dom/manipulation.ts

packages/utils/src/dom/virtualElements.ts

packages/utils/src/object.ts

phillipc · 2025-03-05T13:46:11Z

@brianmhunt: The reactivation of CodeQL worked so well that it directly filled all PRs with warnings. For now, I left it at a trigger via cron and deactiviated it on any PR. We would first have to configure a “meaningful” baseline for CodeQL.

brianmhunt

Some more comments, great progress, thank you! I will return for more

brianmhunt · 2025-03-05T16:12:45Z

packages/binding.if/spec/ifBehaviors.ts

@@ -27,7 +27,8 @@ import {bindings as coreBindings} from '@tko/binding.core'
 import '@tko/utils/helpers/jasmine-13-helper'

 describe('Binding: If', function () {
-  beforeEach(jasmine.prepareTestNode)
+  var testNode : HTMLElement


prefer let to avoid hoisting?

I would fix this globally with eslint in another PR afterwards. It's already prepared in @Auge19's branch.

brianmhunt · 2025-03-05T16:13:13Z

packages/binding.if/spec/elseBehaviors.ts

@@ -26,7 +26,8 @@ import {
 import '@tko/utils/helpers/jasmine-13-helper';

 describe('else inside an if binding', function () {
-  beforeEach(jasmine.prepareTestNode);
+  var testNode : HTMLElement


prefer let (not a blocker, just noting)

brianmhunt · 2025-03-05T16:14:17Z

packages/binding.foreach/src/foreach.ts

@@ -281,7 +318,7 @@ export class ForEachBinding extends AsyncBindingHandler {
    if (as) {
      return v => this._contextExtensions($ctx.extend({ [as]: v }))
    } else {
-      return v => $ctx.createChildContext(v, null, ctx => this._contextExtensions(ctx))
+      return v => $ctx.createChildContext(v, undefined, ctx => this._contextExtensions(ctx))


I assume our tests will cover the consequences of this, and that it's fine, but just noting that it's a somewhat semantic change in fairly fickle code. Probably ok, but noting just in case.

It should be covered. I'm always a bit unsure about this situations in JS. Is there a general rule like "undefinied is better than null" in JS/TS-Styling-Guides?

The general rule is that they are different and fickle and all a bit insane e.g.

undefined == null is true // equality with type-cast undefined === null is false // strictly inequal null + 1 is 1 undefined + 1 is NaN function x (def = 1) { return def } x(null) is null x(undefined) is 1 typeof null is "object" typeof undefined is "undefined"

So it's often dangerous to change old code from null to undefined and vice versa because of these (and many more) subtle differences that may not be immediately apparent, particularly if they're not covered by tests.

If we can keep this as null, that'd be safer.

I looked at the specific location again. The type definition is taken from the official "knockout.d.ts": dataItemAlias?: string The value is also "safely" handled with if (dataItemAlias) L116 at bindingContext.ts.

At this point I would call it a type error in foreach.ts. In general, however, I agree with you, we have to validate something like this case-by-case. Unfortunately JS allowed both. If you like, we can leave it at dataItemAlias?: string | null.

brianmhunt · 2025-03-05T16:19:32Z

packages/binding.foreach/src/foreach.ts

+  deleted: any[]
+}
+
+interface ChangeAddItem{


A stronger type here is:

interface ChangeAddBatchItem { status: 'added' index: number isBatch: true values?: any[] } interface ChangeAddOneItem { status: 'added' index: number isBatch?: false value: any } type ChangeAddItem = | ChangeAddBatchItem | ChangeAddOneItem

then one can typeguard on this i.e.

const c: ChangeAddItem = ... if (c.isBatch) { // c is ChangeAddBatchItem c.values c.value // TypeError } else { // c is ChangeAddOneItem c.value c.values // TypeError }

Not strictly needed, but good to have

https://www.typescriptlang.org/play/?#code/FASwdgLgpgTgZgQwMZQAQGEAWCwHMoCCAJkQEIIRKYCS0AtqgN7CqoDOEFArmwFyoByBCShEBLVOCJQAHvzBc6AI1gSQbcpUz8IMLlAkA3BABt9bAPz8cATwDaAXWABfYKEixEKDNjyESAPJgULRQDMysHNx8gsLSYmpg0nKoCsqqrOqaVFaoiCZsBqzGZlDWYDYubhA2AA5oWDj4xEShDAC8qBIAPj5N-mQUVG09fX4tQSH0bkgA9mAcqEj8jeMkbaidjM6oCGy7FaBwqAAUSAB0WUOYAJRMEgD0D0uS+6vNJNk006wXJfqoJ6oAAqdSgAFEYDBZjAJH9TOZAc9ZgBrFyoKAFNARJEvdRjD5ESYjX7nf5oIGouFkhFQfZA0H1SHQ2HOIA

brianmhunt · 2025-03-05T16:22:00Z

packages/binding.foreach/src/foreach.ts

 //      Utilities
 const MAX_LIST_SIZE = 9007199254740991

 // from https://github.com/jonschlinkert/is-plain-object
-function isPlainObject (o) {
+function isPlainObject (o): o is Object {


The canonical object type is o is Record<string, any>, since Object is the parent of Array, Function, and many other classes.

brianmhunt · 2025-03-05T16:24:38Z

packages/binding.core/src/textInput.ts

-  watchForSelectionChangeEvent (element, ieUpdateModel) {
+
+  // All variables are not found!
+  watchForSelectionChangeEvent (element?) {


it's unclear what's happening here and it doesn't seem related to the change of types so we might consider a separate PR to make sure the implications are well understood.

Could we break this particular change into a separate PR?

At this point we noticed through the typing that the old IE 8/9 logic could not have worked at all. We made it tsc compliant for now and added a FIXME maker.

I think that we can in a later separate pr, remove all IE8 to IE10-Logic. IE11 could be discussed.

element can be undefinied , handler is never defined, ieUpdateModel will not be handed over

brianmhunt · 2025-03-05T16:26:23Z

packages/binding.core/src/let.ts

 export default {
-  init: function (element, valueAccessor, allBindings, viewModel, bindingContext) {
+  init: function (element, valueAccessor, allBindings, viewModel, bindingContext: BindingContext) { // allBindings and viewModel not used!


We could use the convention to start unused vars with _ e.g.

init: function (element, valueAccessor, _allBindings, _viewModel, bindingContext: BindingContext) {

then the comment isn't needed and in many editors the variable will have a different color for the unused var

@M-Kirchhoff has this adapted.

Actually, we should collect such things in a general TKO style guide.

brianmhunt

I'm up to 55/193 files, making progress. Thank you!

brianmhunt · 2025-03-06T13:48:48Z

packages/bind/spec/bindingDependencyBehaviors.ts

@@ -33,7 +33,8 @@ import '@tko/utils/helpers/jasmine-13-helper';
 describe('Binding dependencies', function () {
  var bindingHandlers

-  beforeEach(jasmine.prepareTestNode);
+  var testNode : HTMLElement


Done, changed in 44 files ;) Let's do the rest later with eslint, it can be automated.

brianmhunt · 2025-03-06T13:51:46Z

packages/bind/src/bindingContext.ts

-      self.$parent = parentContext.$data
-      self.$parents = (parentContext.$parents || []).slice(0)
+      self.$parent = parentContext?.$data
+      self.$parents = (parentContext?.$parents || []).slice(0)


I assume $parents cannot be a falsy e.g. false or 0, but to be safe it's generally better to use ...?.$parents ?? []

(not needed, just noting)

brianmhunt · 2025-03-06T13:52:25Z

packages/bind/src/bindingContext.ts

@@ -147,30 +196,30 @@ Object.assign(bindingContext.prototype, {
  extend (properties) {
    // If the parent context references an observable view model, "_subscribable" will always be the
    // latest view model object. If not, "_subscribable" isn't set, and we can use the static "$data" value.
-    return new bindingContext(inheritParentIndicator, this, null, function (self, parentContext) {
+    return new bindingContext(inheritParentIndicator, this, undefined, function (self, parentContext) {


I'm assuming this has no material difference, but it's semantic and not just a type change, so we want to be mindful of knock-on consequences.

Link to packages/binding.foreach/src/foreach.ts discussion

brianmhunt · 2025-03-06T13:56:57Z

packages/bind/src/bindingContext.ts

  // We can only do something meaningful for elements and comment nodes (in particular, not text nodes, as IE can't store domdata for them)
  if (node && (node.nodeType === 1 || node.nodeType === 8)) {
    return storedBindingContextForNode(node)
  }
 }

-export function dataFor (node) {
+export function dataFor<T = any>(node: Node): T | undefined {


I'm not clear on what this generic does. Which is ok, and maybe I'll see it when it's used, but just noting.

Comes from the official d.ts files.. Only for the return value relevant. We intended to keep things as similar as possible. In reality, we had to compromise to the current state of implementation.

With make dts we can already generate definitions quite well. We would just need a delivery process later.

brianmhunt · 2025-03-06T13:58:56Z

packages/binding.core/spec/eventBehaviors.ts

-    triggerEvent(testNode.childNodes[0], 'click')
+    triggerEvent(testNode.children[0], 'click')
+    triggerEvent(testNode.children[0], 'click')
+    triggerEvent(testNode.children[0], 'click')


I think all these .children[0] are ok replacing .childNodes[0]. The only case where there'd be a false positive is if there were two nodes and .childNodes[0] !== .children[0] and clicking on the .children[0] created a false positive. That doesn't seem likely here, so I'm ok with it, just noting.

alright, triggerEvent needs an element

brianmhunt · 2025-03-06T14:22:51Z

packages/binding.foreach/src/foreach.ts

@@ -281,7 +318,7 @@ export class ForEachBinding extends AsyncBindingHandler {
    if (as) {
      return v => this._contextExtensions($ctx.extend({ [as]: v }))
    } else {
-      return v => $ctx.createChildContext(v, null, ctx => this._contextExtensions(ctx))
+      return v => $ctx.createChildContext(v, undefined, ctx => this._contextExtensions(ctx))


The general rule is that they are different and fickle and all a bit insane e.g.

undefined == null is true // equality with type-cast undefined === null is false // strictly inequal null + 1 is 1 undefined + 1 is NaN function x (def = 1) { return def } x(null) is null x(undefined) is 1 typeof null is "object" typeof undefined is "undefined"

So it's often dangerous to change old code from null to undefined and vice versa because of these (and many more) subtle differences that may not be immediately apparent, particularly if they're not covered by tests.

If we can keep this as null, that'd be safer.

phillipc · 2025-03-07T09:57:12Z

packages/bind/spec/arrayToDomEditDetectionBehaviors.ts

@@ -33,29 +27,29 @@ describe('Array to DOM node children mapping', function () {
    }
    setDomNodeChildrenFromArrayMapping(testNode, array, mapping)
    expect(testNode.childNodes.length).toEqual(4)


@brianmhunt This is also a useful methodology to deal with the assertion problem in the ChildNodes/Children adaptation.

…jsdocs because its our top api

brianmhunt

Ok I'm back to 37/193 files reviewed, so we're actually going in reverse here. :)

Github is not good at these huge PR reviews. Is there any way we could create a series of PRs based on the changes here?

Having looked through a lot, much of it seems safe, but we want to a comprehensive second set of eyes just in case. e.g. can you pick 5-10 files and create a PR for those? I'm open to other strategies, but getting this very-large PR merged all-at-once may be a challenge.

The only general change that I think I'd like to discuss is using new Array() instead of []. How strong are you on that / are there alternatives?

Just as a rubber duck I asked Claude AI why one would prefer new Array() over [] and it somewhat aligned with my own thinking. I might be missing something and am open to discussion, but it was on my mind so wanted to investigate / raise it.

Thank you!

phillipc · 2025-03-11T20:34:12Z

@brianmhunt I think we going in reserve with the review because of doing changes while you reviewing it. The last suggested const-to-let-convertation hits 44 files. I suggest we hold off on further changes to this PR until your review is complete. We are currently very motivated and would like to achieve 100% TSC conformity with this PR so that further improvements to TKO are possible on this basis.

Regarding the array topic, it is a tick for now so that the type system works: push, length and co are defined like an any[]. Later, all arrays should be clearly declared: number[] or Array<number>(). ~~This leads to the same result, number[] is shorter.~~

Update 1: [] is an never[] so we can only push never-types, see https://stackoverflow.com/questions/52423842/what-is-not-assignable-to-parameter-of-type-never-error-in-typescript

Update 2: In fact, these are different array variants. The following article examines this. https://medium.com/@julienetienne/different-types-of-arrays-in-javascript-and-when-to-use-them-77f7843b71de
I have also created a playground for this topic.

Eslint would also find this flaw if a length is not defined: https://eslint.org/docs/latest/rules/no-array-constructor - eslint can partly fix this automatically, but... i would like to leave the topic as it is in this PR and tackle it in a second step together with the ESLINT installation. It generates 156 new warnings, which we would then correct by defining the array in a type-safe manner as number[] or something similar. Can we stay this way? A convert to typeof any[] would be another solution..

brianmhunt · 2025-03-12T11:22:48Z

@brianmhunt I think we going in reserve with the review because of doing changes while you reviewing it. The last suggested const-to-let-convertation hits 44 files. I suggest we hold off on further changes to this PR until your review is complete. We are currently very motivated and would like to achieve 100% TSC conformity with this PR so that further improvements to TKO are possible on this basis.

Ok that should work, as long as this branch stays frozen (and we can do PRs into this branch, but wait to merge them), this should prevent the need to re-review. If there are things to fix in this PR we can decide whether it happens here or in a chained PR.

Regarding the array topic, it is a tick for now so that the type system works: push, length and co are defined like an any[]. Later, all arrays should be clearly declared: number[] or Array<number>(). ~~This leads to the same result, number[] is shorter.~~

Update 1: [] is an never[] so we can only push never-types, see https://stackoverflow.com/questions/52423842/what-is-not-assignable-to-parameter-of-type-never-error-in-typescript

Update 2: In fact, these are different array variants. The following article examines this. https://medium.com/@julienetienne/different-types-of-arrays-in-javascript-and-when-to-use-them-77f7843b71de I have also created a playground for this topic.

Eslint would also find this flaw if a length is not defined: https://eslint.org/docs/latest/rules/no-array-constructor - eslint can partly fix this automatically, but... i would like to leave the topic as it is in this PR and tackle it in a second step together with the ESLINT installation. It generates 156 new warnings, which we would then correct by defining the array in a type-safe manner as number[] or something similar. Can we stay this way? A convert to typeof any[] would be another solution..

Thanks for the summary, I see it's the noImplicitAny and strictNullChecks, which are both very useful here.

Leaving them in works for me, it'll be easy to find the new Array() b/c they're inherently findable. The only real cost is the additional bytes and weak typing from new Array, which we can in the future fix with const x: something[] = [], if we feel it's necessary. Thanks for walking through it.

I'll continue on in the review of this PR.

Thanks @phillipc

For future reference:

brianmhunt

I'm up to 70/193 files reviewed. Thank you!

brianmhunt · 2025-03-12T11:24:06Z

packages/bind/spec/bindingAttributeBehaviors.ts

@@ -26,11 +26,14 @@ import { bindings as templateBindings } from '@tko/binding.template'
 import { bindings as ifBindings } from '@tko/binding.if'

 import '@tko/utils/helpers/jasmine-13-helper'
+import { Provider } from '@tko/provider'


is this used? maybe import type { Provider } ...?

That's so right, see L756.

brianmhunt · 2025-03-12T11:27:06Z

packages/bind/src/applyBindings.ts

    for (const [key, BindingHandlerClass] of bindingsGenerated) {
        // Go through the sorted bindings, calling init and update for each
-      function reportBindingError (during, errorCaptured) {
+      const reportBindingError = function (during, errorCaptured) {


it seems odd to make this anonymous, so it won't report in stack traces. probably ok, though.

IMO such a function should even better be anonymous and const, otherwise there may be problems with the scope. https://stackoverflow.com/questions/750486/javascript-closure-inside-loops-simple-practical-example

brianmhunt · 2025-03-12T11:31:52Z

packages/binding.core/spec/enableDisableBehaviors.ts

@@ -29,33 +30,38 @@ describe('Binding: Enable/Disable', function () {
    var myObservable = observable()
    testNode.innerHTML = "<input data-bind='enable:myModelProperty()' />"
    applyBindings({ myModelProperty: myObservable }, testNode)
-
-    expect(testNode.childNodes[0].disabled).toEqual(true)
+    var input = testNode.children[0] as HTMLInputElement


should be const. small thing, but just to prevent hoisting since it's used a bunch in the other functions here where it could impact outcomes

Done in Auge19#4

brianmhunt · 2025-03-12T11:33:22Z

packages/binding.core/spec/selectedOptionsBehaviors.ts

@@ -40,8 +41,8 @@ describe('Binding: Selected Options', function () {

  it('Should set selection in the SELECT node to match the model', function () {
    var bObject = {}
-    var values = new observableArray(['A', bObject, 'C'])
-    var selection = new observableArray([bObject])
+    var values = observableArray(['A', bObject, 'C'])


should be const (not a blocker here, just noting)

brianmhunt · 2025-03-12T11:33:53Z

packages/binding.core/spec/styleBehaviors.ts

@@ -28,70 +29,76 @@ describe('Binding: CSS style', function () {
  })

  it('Should be able to use standard CSS style name (rather than JavaScript name)', function () {
-    var myObservable = new observable('red')
+    var myObservable = observable('red')


also let. I'll just note that generally and we can review/revisit in the future, it's not blocking, just noting.

brianmhunt · 2025-03-12T11:34:56Z

packages/binding.core/spec/usingBehaviors.ts

@@ -183,7 +186,7 @@ describe('Binding: Using', function () {
    testNode.innerHTML = "<div data-bind='using: someitem'>text" +
            "<!-- ko foreach: childprop --><span data-bind='text: $data'></span><!-- /ko --></div>"

-    var childprop = observableArray([])
+    var childprop: ObservableArray = observableArray([])


weird that the : ObservableArray is needed, I'd expect observableArray([]) to provide that

It is not necessary, but its results in ObservableArray<never>.

Right at this point would be var childprop = observableArray<string>([]). The current way results in ObservableArray<any>.

Done, changed to generic in Auge19#4

phillipc · 2025-03-12T13:38:35Z

Thanks @brianmhunt for the current process and progress. I have collect some fruther task here: #211 . I would be very interested in your thoughts on this points.

brianmhunt

👍 77/193

brianmhunt · 2025-03-17T19:12:09Z

tsconfig.json

-    "noEmit": true,
+    "noEmit": true, //Deactivate "noEmit" to generating d.ts-File to 'builds/dts/dist' via "make dts"
+    "emitDeclarationOnly": true,
+    "outDir": "builds/dts/dist",


I feel like the builds/dts/dist may change depending on whether it's the knockout or the tko build, but also it's ok for now - just noting.

This is just an poc for now. The normal build chain is not affected by these parameters. I'm still unclear on how best to build a typing package. Once the PR is complete, I'd like to try it with https://www.npmjs.com/package/esbuild-plugin-d.ts

Regarding the build pipeline, I would preferred something like a one file bundle for TKO and @types/tko, with an esm export: export var tko. Corresponds to browser.es6.js

Thanks, I'm not clear on the best build strategy either. It'd be nice if the only dependency were Typescript itself, for the typing.

On the @types/tko we should be ok, noting that the static global for the knockout and the tko build may be different, so we want to be mindful of that.

brianmhunt · 2025-03-17T19:12:26Z

tsconfig.json

+    "noEmit": true, //Deactivate "noEmit" to generating d.ts-File to 'builds/dts/dist' via "make dts"
+    "emitDeclarationOnly": true,
+    "outDir": "builds/dts/dist",
+    /*Disable some typing feature from ts to detect first steps to a strong typed version*/


brianmhunt · 2025-03-17T19:14:38Z

packages/utils/src/string.ts

+        return JSON.parse(jsonString) as T;
+      }
+      // Fallback on less safe parsing for older browsers
+      return (new Function('return ' + jsonString))() as T


Noting (aside) we should really rely on JSON as a polyfill, if it's not defined.

This new Function(string) is like evil eval.

Good point, probably this case is generally obsolete. I think it is for IE < 8 and old Safari browser.. https://caniuse.com/json

yeah I think it's safe/wise to pull this out. It opens a vector to arbitrary code execution - if someone can unset the global JSON the fallback is dangerous. We're relying on newer things than JSON, so this won't be the thing that breaks TKO.

brianmhunt · 2025-03-17T19:16:52Z

packages/utils/src/options.ts

-  set: function (name, value) {
-    options[name] = value
-  },
+  onError (e : Error) : void { throw e }


stylistically, the x:type is preferable to x : type, the latter looking a bit too much like a ternary. (not a blocker, just noting)

brianmhunt · 2025-03-17T19:27:49Z

packages/utils/src/options.ts


    // jQuery will be automatically set to globalThis.jQuery in applyBindings
    // if it is (strictly equal to) undefined.  Set it to false or null to
    // disable automatically setting jQuery.
-  jQuery: globalThis.jQuery,
+  jQuery : JQueryStatic | boolean | null = (globalThis as any).jQuery


I'm not sure it can ever be true, just false

jQuery : JQueryStatic | false | null = (globalThis as any).jQuery

Nothing needed to change here, just noting

Fixed in Auge19#4

brianmhunt · 2025-03-17T19:28:24Z

packages/utils/src/options.ts

 //
 // This becomes ko.options
 // --
 //
 // This is the root 'options', which must be extended by others.
+export class Options {
+  [key: string]: any;


do we want this? doesn't that make all the other types any?

The declared ones win's, but I would preferred to remove the index access here. We have some hidden options in the codebase. Declaring those is the better way.

Fixed in Auge19#4

brianmhunt · 2025-03-17T19:28:47Z

packages/utils/src/options.ts

+import { Provider } from "@tko/provider";
+
+interface CustomBindingGlobalProperties {
+  String;


unclear what String is

It was wrong, fixed it Auge19#4

interface CustomBindingGlobalProperties { [customBindingName: string]: any; }

String and isObservable are only used as test-case-props in templatingBehaviors.ts

brianmhunt · 2025-03-17T19:29:35Z

packages/utils/src/object.ts

-export function clonePlainObjectDeep (obj, seen) {
-  if (!seen) { seen = [] }
+export function clonePlainObjectDeep (obj, seen?: any[]) {
+  if (!seen) { seen = new Array() }


aside: we could/should improve the perf. here with a Set instead of array for seen

I took a look at the method and it looks like it's not used anymore. Mark as deprecated?

Yes, even just /** @deprecated */ (in a subsequent PR) would be nice here. It's possible someone is using it externally but it's not a core deliverable of TKO/knockout and trivial to do on your own so I think it'd be good to get rid of it if it's not being used internally.

brianmhunt

👍 115/193

brianmhunt · 2025-03-18T11:31:10Z

packages/binding.template/spec/templatingBehaviors.ts

@@ -174,21 +177,21 @@ describe('Templating', function () {
    setTemplateEngine(new dummyTemplateEngine({ someTemplate: 'template output' }))
    testNode.innerHTML = "<div data-bind='template:\"someTemplate\"'></div>"
    applyBindings(null, testNode)
-    expect(testNode.childNodes[0].innerHTML).toEqual('template output')
+    expect((testNode.childNodes[0]as HTMLElement).innerHTML).toEqual('template output')


small thing: whitespace before as

oh, fixed in Auge19#4

brianmhunt · 2025-03-18T11:34:14Z

packages/binding.template/src/templating.ts


-      elseChainSatisfied((unwrap(dataArray) || []).length !== 0)
+      elseChainSatisfied((unwrap(dataArray) || []).length !== 0);


it'd be nice if the style guide here highlighted and ideally removed unnecessary semicolons

I've already prepared prettier here. It's configured to maintain the existing style: https://github.com/Auge19/tko/tree/add_prettier

Personally, I would prefer globaly ending semicolons.

brianmhunt · 2025-03-18T11:35:44Z

packages/binding.template/src/templating.ts

@@ -171,7 +179,7 @@ export function renderTemplate (template, dataOrBindingContext, options, targetN
        // Ensure we've got a proper binding context to work with
        var bindingContext = (dataOrBindingContext && (dataOrBindingContext instanceof BindingContextConstructor))
              ? dataOrBindingContext
-              : new BindingContextConstructor(dataOrBindingContext, null, null, null, { 'exportDependencies': true })
+              : new BindingContextConstructor(dataOrBindingContext, undefined, undefined, undefined, { 'exportDependencies': true })


possibly semantically relevant i.e. undefined !== null. Likely ok, but noting.

brianmhunt · 2025-03-18T11:38:09Z

packages/computed/spec/asyncBehaviors.ts

@@ -56,7 +56,7 @@ describe('Throttled observables', function () {
        // Wait until after timeout
    waitsFor(function () {
      return notifiedValues.length > 0
-    }, 300)
+    }, "Timeout", 300)


it would be stronger to declare const waitsFor: (...) => ...

waitsFor from Jasmine 1.3 is only declared with this three parameters:

declare function waitsFor(latchMethod: () => boolean, failureMessage?: string, timeout?: number): void;

Adding a creative failure messages like "Timeout" is compliant with the jasmine typings, although this magic always happens:

jasmine.Spec.prototype.waitsFor = function(latchFunction, optional_timeoutMessage, optional_timeout) { var latchFunction_ = null; var optional_timeoutMessage_ = null; var optional_timeout_ = null; for (var i = 0; i < arguments.length; i++) { var arg = arguments[i]; switch (typeof arg) { case 'function': latchFunction_ = arg; break; case 'string': optional_timeoutMessage_ = arg; break; case 'number': optional_timeout_ = arg; break; } } var waitsForFunc = new jasmine.WaitsForBlock(this.env, optional_timeout_, latchFunction_, optional_timeoutMessage_, this); this.addToQueue(waitsForFunc); return this; };

brianmhunt · 2025-03-18T11:40:04Z

packages/computed/spec/asyncBehaviors.ts

      expect(function () {
-        observable.extend({deferred: false})
+        observable.extend(Object.assign({deferred: false}))


This is a no-op. i.e. Object.assign isn't doing anything here. It's harmless, but worth removing (either in this PR or the next)

Done, Auge19#4

brianmhunt · 2025-03-18T11:46:04Z

packages/filter.punches/src/index.ts

-// Convert value to uppercase
-filters.uppercase = function (value) {
-  return sproto.toUpperCase.call(unwrap(value))
+interface Filters {


many consumers of Knockout/TKO will extend the filters, so this type has to be exposed for others to extend it.

export is added, see Auge19#4

brianmhunt · 2025-03-18T11:47:13Z

packages/filter.punches/src/index.ts

This looks to mostly be re-organizing, and I didn't spot any semantic changes but it was a lot of changes so I may have missed something. Please let me know if there were any semantic changes here, but otherwise it's all good.

I have check this, too. Only the implementation style is changed.

brianmhunt · 2025-03-18T11:47:57Z

packages/lifecycle/spec/LifeCycleBehaviors.ts

-} from '../dist'
+} from '../src'
+
+import { assert } from 'chai';


import order should have relative imports after vendor/absolute i.e. 'chai' before '../src'

Changed in Auge19#4 . It is only a stylistic pattern or is there another reason?

brianmhunt · 2025-03-18T11:49:06Z

packages/lifecycle/spec/LifeCycleBehaviors.ts

@@ -48,11 +50,12 @@ describe('KO LifeCycle', function () {
      assert.isFunction(Child.prototype.anchorTo)
      assert.isFunction(Child.prototype.dispose)
      assert.isFunction(Child.prototype.addDisposable)
-      assert.isNotFunction(Child.prototype.mixInto)
+      // TODO: Fails with tsc, check if test is obsolte with typescript
+      // assert.isNotFunction(Child.prototype.mixInto)


Noted. Since it's unlikely to be used and a strange/rare pattern, this feels ok to me.

Alternatively we could use any or set ts-ignore for the isNotFunction check?

brianmhunt · 2025-03-18T11:52:03Z

packages/observable/src/mappingHelpers.ts


        // For arrays, also respect toJSON property for custom mappings (fixes #278)
-    if (typeof rootObject['toJSON'] === 'function') { visitorCallback('toJSON') }
+  if (typeof rootObject['toJSON'] === 'function') { visitorCallback('toJSON') }


wrong indent?

Fixed in Auge19#4

brianmhunt

up to 175/193 . Thanks for all the hard work here.

I've added a few comments, some are meaningful, but nothing strictly blocking.

The remaining files tend to be larger ones, so may take a bit each, but will turn to it as soon as I can. Thank you!

brianmhunt · 2025-03-31T12:29:42Z

packages/provider.component/src/ComponentProvider.ts

    if (node.tagName === 'SLOT') {
      const parent = node.parentNode
      const slotName = node.getAttribute('name') || ''
      const openNode = document.createComment(`ko slot: "${slotName}"`)
      const closeNode = document.createComment('/ko')
+
+      if(!parent)


should use if (...) { ... } by convention / clarity / lint

(not a blocker, but noting)

brianmhunt · 2025-03-31T12:30:36Z

packages/provider.databind/spec/dataBindProviderBehaviors.ts

@@ -38,6 +38,9 @@ import {

 import * as coreBindings from '@tko/binding.core';

+import { assert} from "chai"


these vendor imports should be above the "relative" imports (not a blocker)

Done in Auge19#4, i have also fixed unneeded imports and testseries closure. I think everything is only stylistic icing on the cake.

brianmhunt · 2025-03-31T12:31:08Z

packages/provider.multi/spec/MultiProviderBehaviors.ts

@@ -3,6 +3,8 @@ import {
  MultiProvider
 } from '../dist'

+import { assert } from "chai"


should be before relative imports

Done, Auge19#4

brianmhunt · 2025-03-31T12:31:52Z

packages/provider.mustache/helpers/jasmine-interpolation-helpers.ts

unclear in review whether it's ok to delete this, just noting. I imagine if the tests still run it's fine, but noting.

I have merge this file with the central file: utils/helpers/jasmine-13-helper.ts. All globally declared custom matchers should be always available.

brianmhunt · 2025-03-31T12:33:03Z

packages/provider.mustache/spec/textInterpolationSpec.ts

@@ -251,7 +251,7 @@ describe('Interpolation Markup bindings', function () {

    jasmine.setNodeText(testNode, "hello {{'name'}}!");
    applyBindings(null, testNode);
-    expect(testNode).toContainText('hello --name--!');
+    expect(testNode).toContainText('hello --name--!'); */


are we removing a test expect here? do we lose some coverage?

This test-case was already deactivited via xit, so we saved ourselves the type correction.

brianmhunt · 2025-03-31T12:39:36Z

packages/utils.component/src/registry.ts

-} from '@tko/utils'
+import { subscribable, dependencyDetection } from '@tko/observable'
+import { getObjectOwnProperty, tasks } from '@tko/utils'
+import { Loader } from './loaders'


import type

Done, see Auge19#4

brianmhunt · 2025-03-31T12:40:13Z

packages/utils.jsx/spec/jsxBehaviors.ts

  // For testing purposes, we make this synchronous.
  detachAndDispose (node) {
    super.detachAndDispose(node)
    cleanNode(node)
  }
+
+  dispose () {


this is a no-op

packages/utils/index.ts

brianmhunt · 2025-03-31T12:45:04Z

packages/utils/spec/memoizationBehaviors.ts

@@ -1,6 +1,8 @@

 import { memoization } from '../dist'

+//import { } from "karma-jasmine";


Done, see Auge19#4

brianmhunt · 2025-03-31T12:45:52Z

packages/utils/src/memoization.ts

@@ -52,7 +53,9 @@ export function unmemoizeDomNodeAndDescendants (domNode, extraCallbackParamsArra
  }
 }

-export function parseMemoText (memoText) {
+export function parseMemoText (memoText : string | null) : string | null {
+  if(!memoText)


needs if () { ... }

Done, see Auge19#4

phillipc and others added 30 commits December 20, 2024 21:45

configure typescript

18ea310

add some types

+resolved the frist 1000 warnings

91eb802

#we should remove jasmine..

add some global types

2b994e4

switch d.ts-version for jasmine (the project used jasmine 1.3)

fix: check text/textcontent for comments / add test

2b40fa0

fix some typescript warnings

f105406

fix some types:

11fd46e

1) [] => new Array 2) childNodes => children 3) extend some global type-defs

Merge branch 'dockercontainer_and_fixed_deps_for_testing' into strong…

617c335

…_typed

Readme) some updates for the current stack

4b0b47f

Observable Typings ...

4a25a08

Fixed TSC errors in utils

1081dfb

Utils.Parser fixed tsc error

b4f4e98

Fixed Typo

b47a40a

fix tsc error

8b53a0e

Merge branch 'knockout:main' into strong_typed

d0b3ee6

Merge remote-tracking branch 'origin/fixbrokenTest' into strong_typed

f3abdd4

TS)

4c95467

fix wrong jasmine imports fix method signature

ts) add assertion

5c1be03

Merge branch 'strong_typed' of https://github.com/Auge19/tko into str…

cdcc9e4

…ong_typed

fixing tsc errors

5838247

functionrewrite fix

309b9f9

more tsc fixes

26b3f73

adding props to bindinghandler

9ceaf34

Merge branch 'strong_typed' of https://github.com/Auge19/tko into str…

44b374b

…ong_typed

fixed bindingDependencyBehaviors

a1124a2

utils.component fixes

ed5e0cd

componentBindingBehaviour tsc fixed

0392314

tsc fixes

c9deeff

Merge commit '03923144a2ecb72fc8d41150c1707ae8badf4d24' into strong_t…

1324f71

…yped

foreach tsc fixed

a08a992

slotBinding tsc fixed

b978ee0

phillipc added 2 commits March 3, 2025 17:01

ts) revert to childNodes

b8e996f

ts) some reverts to childNodes

b620b3a

brianmhunt reviewed Mar 4, 2025

View reviewed changes

Merge branch 'knockout:main' into strong_typed

fec5a16

github-advanced-security bot found potential problems Mar 4, 2025

View reviewed changes

Merge branch 'knockout:main' into strong_typed

695f9d1

phillipc added 3 commits March 5, 2025 14:50

ts) use lamda to get input element from dom

2ec5641

ts) BUILD_VERSION as const

956c6d5

ts) better stack trace with named functions

9d7ffb3

brianmhunt reviewed Mar 5, 2025

View reviewed changes

ts) typings

56d6ada

brianmhunt reviewed Mar 6, 2025

View reviewed changes

phillipc added 2 commits March 6, 2025 21:11

ts) var to let in test-cases

b6bf565

ts) safer fallback

87def81

phillipc commented Mar 7, 2025

View reviewed changes

phillipc added 5 commits March 7, 2025 11:58

ts) better assertions

08cd09e

update readme for a better look

e5378b7

readme) updates / revert electron-deps

c389803

readme) add debugging hint

76797d2

ts) fix two type-def-errors in observable (subprop/value as T) / add …

53983f8

…jsdocs because its our top api

brianmhunt reviewed Mar 11, 2025

View reviewed changes

brianmhunt reviewed Mar 12, 2025

View reviewed changes

phillipc mentioned this pull request Mar 15, 2025

ts) Fixes for Branch 'stong_typed' Auge19/tko#4

Open

brianmhunt reviewed Mar 17, 2025

View reviewed changes

brianmhunt reviewed Mar 18, 2025

View reviewed changes

brianmhunt reviewed Mar 31, 2025

View reviewed changes


		elseChainSatisfied((unwrap(dataArray) \|\| []).length !== 0)
		elseChainSatisfied((unwrap(dataArray) \|\| []).length !== 0);

		@@ -38,6 +38,9 @@ import {

		import * as coreBindings from '@tko/binding.core';

		import { assert} from "chai"

		@@ -1,6 +1,8 @@

		import { memoization } from '../dist'

		//import { } from "karma-jasmine";

Typescript) fix errors #202

Are you sure you want to change the base?

Typescript) fix errors #202

Conversation

phillipc commented Jan 12, 2025 • edited Loading

brianmhunt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phillipc commented Mar 5, 2025

brianmhunt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phillipc Mar 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianmhunt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianmhunt left a comment

Choose a reason for hiding this comment

phillipc commented Mar 11, 2025 • edited Loading

brianmhunt commented Mar 12, 2025 • edited Loading

brianmhunt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phillipc commented Mar 12, 2025

brianmhunt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phillipc Mar 18, 2025 • edited Loading

Choose a reason for hiding this comment

phillipc commented Jan 12, 2025 •

edited

Loading

phillipc Mar 6, 2025 •

edited

Loading

phillipc commented Mar 11, 2025 •

edited

Loading

brianmhunt commented Mar 12, 2025 •

edited

Loading

phillipc Mar 18, 2025 •

edited

Loading