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

A few more comments, looking really great. The main concern I've spotted is the loss of info changing assertions from .childNodes to .children. Would be grateful for some thoughts on that. Thank you, and I'll keep turning back to look at this

brianmhunt · 2025-03-03T12:32:36Z

builds/knockout/src/index.ts

-      new AttributeProvider()
-    ]
-  }),
+  provider: provider,


can use the shorthand provider, in place of provider: provider,, which would be consistent with filters, below, and in any case shorter. Not needed, just noting

brianmhunt · 2025-03-03T12:34:03Z

builds/reference/src/index.ts

@@ -52,6 +54,7 @@ const builder = new Builder({
  ]
 })

+// @ts-ignore: Build-Parameter


Perhaps this would work, stronger than an ignore?

declare const BUILD VERSION: string

brianmhunt · 2025-03-03T12:36:20Z

global.d.ts

+        o: () => ObservableArray // For jsxBehaviors Test
+    }
+
+    //Jasmine and Mocha define duplicated functions, is a problem for the type system


yes, those or playwright.dev or bun.sh. They're pretty tightly tied to Jasmine right now, so it'd be an effort, but homogenizing the runner would help a lot.

packages/bind/spec/bindingAttributeBehaviors.ts

brianmhunt · 2025-03-03T12:41:25Z

packages/bind/spec/asyncBindingBehaviors.ts

@@ -244,14 +244,15 @@ describe('Deferred bindings', function () {

      // Value is initially true, so nodes are retained
    applyBindings({ someItem: someItem, counter: 0 }, testNode)
-    expect(testNode.childNodes[0].childNodes[0].tagName.toLowerCase()).toEqual('span')
+
+    expect(testNode.children[0].children[0].tagName.toLowerCase()).toEqual('span')


ah right, typing. I wonder if .childNodes, being broader, is a stronger test and we are introducing the possibility of false negative tests here i.e. missing failures where childNodes would have additional information that would reveal failures.

There are a few cases to consider:

.childNodes having text/virtual nodes that it shouldn't and the test would fail, but .children test would still pass

.children ignores/collapses important white-space that .childNodes picks up

Which is all to say, for the sake of type safety this would make the tests weaker and if/as we change TKO we may introduce bugs that the stronger variants may pick up.

Not to say I'm entirely opposed to the change, but wondering if perhaps there's an alternative

testNode.childNodes[0].childNodes[0]['tagName']['toLowerCase']()

Properly there'd be type guarding in place, but that'd be a heftier ask I feel.

Any thoughts come to mind on this concern?

brianmhunt · 2025-03-03T12:54:35Z

packages/bind/src/bindingContext.ts

+}
+
+export interface BindingContext<T = any> {
+  ko: any; // typeof ko;


not needed here, but it would be nice to even have a placeholder like KnockoutInternalInstance = any so we could track these down later and strengthen the types

Good point, done!

brianmhunt · 2025-03-03T12:56:27Z

packages/bind/src/bindingContext.ts

 // The bindingContext constructor is only called directly to create the root context. For child
 // contexts, use bindingContext.createChildContext or bindingContext.extend.
-export function bindingContext (dataItemOrAccessor, parentContext, dataItemAlias, extendCallback, settings) {
+export const bindingContext = function<T>(dataItemOrAccessor: any, parentContext?: BindingContext, dataItemAlias?: string, extendCallback?: BindingContextExtendCallback<T>, settings?: BindingContextSetting) {


making the function anonymous means the name won't show up in stack traces. I'm not sure but we might be able to set bindingContext.name = 'bindingContext' (or perhaps Object.defineProperty, but same idea) and then it's show up in stack traces, but I've never tested this.

I will check this. This style of implementation allows the as unknown as bindingContext magic, so the returned "objects" can be typed. If we convert this to proper TS/JS-classes in the future, this would be solved anyway.

Thanks. It's not a blocker IMO, just an observation since the stack trace is pretty well named already.

I have been analyzing Knockout.js applications since 2015, this is an important point 💯

I found an easy solution in Typescript:

export const bindingContext = function bindingContextFactory<T>(....)

This results in JS:

export const bindingContext = function bindingContextFactory(dataItemOrAccessor, parentContext, dataItemAlias, extendCallback, settings)

The name just has to be different, otherwise tsc will remove it!

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.

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.

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.

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

ts) fix tsc error in builder-usages

21c7978

phillipc mentioned this pull request Feb 28, 2025

Initial Typescriptification of tko.utils #62

Open

mcselle mentioned this pull request Feb 28, 2025

Fix for hasFocus / checked binding if not applied via DataBindProvider #208

Open

phillipc mentioned this pull request Feb 28, 2025

Update build process #111

Open

ts) add type to bindingContext-Factory, use generics

7bff82a

phillipc mentioned this pull request Mar 2, 2025

fix) recovering jquery was broken #207

Merged

brianmhunt reviewed Mar 3, 2025

View reviewed changes

phillipc added 6 commits March 3, 2025 16:27

ts) some fixes after review

d749194

ts) consistent params handover

16988c5

Merge branch 'knockout:main' into strong_typed

65a697b

ts) sync with knockout#207

489914c

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 3 commits March 7, 2025 11:58

ts) better assertions

08cd09e

update readme for a better look

e5378b7

readme) updates / revert electron-deps

c389803

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

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

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

phillipc commented Jan 12, 2025 •

edited

Loading

phillipc Mar 6, 2025 •

edited

Loading