TypeScript-ify #92

caseyWebb · 2019-03-15T04:27:16Z

Includes #62, rebased w/ latest tko master, build process fixed.

Note: I haven't actually reviewed the work by @Sebazzz, nor tried anything in the browser. This is just bringing that PR up to date and adding build support.

TODO

get tests passing
finish refactoring into TS
replace build process with something simpler

… in rollup

mbest · 2019-03-15T17:55:02Z

Some comments:

All functions should have a typed return value.
Don't use Function as a type.
For compatibility with Knockout 3.5.0, exported types should match https://github.com/knockout/knockout/blob/master/build/types/knockout.d.ts

brianmhunt · 2019-03-15T18:02:04Z

Thanks @mbest — what's the reason for not using Function as a type?

Sebazzz · 2019-03-15T18:03:46Z

It is not typed, you don't provide types for input parameters and output type. It's like there can be a function that get any parameters of `any` and might or might not return anything. It does not document anything. If you want to accept a function that does not accept anything and does not return anything you wouldn't use Function either. This is almost never what you want.

brianmhunt · 2019-03-15T19:22:24Z

Thanks @Sebazzz — I'm not sure I understand. Do you mean that Function is untyped, but the correct thing to do in TS is to have types for the function (e.g. someFunction: (n: number) => any)?

Sebazzz · 2019-03-15T19:25:20Z

Yes, but you can model it as a type alias or interface for reusability and clarity if suitable.

For instance:

type SubscriptionCallback<T> = (newValue : T) => void;

// […]

interface Xxx<T> {
    void subscribe(callback: SubscriptionCallback<T>);
}

brianmhunt · 2019-03-16T01:38:28Z

packages/utils/src/dom/disposal.ts

@@ -96,8 +96,7 @@ export function cleanNode (node: Node) {

    // ... then its descendants, where applicable
    if (cleanableNodeTypesWithDescendants[node.nodeType]) {
-      // cleanNodesInList((node as CleanableNodeWithDescendants).getElementsByTagName("*"))
-      cleanNodesInList((node as CleanableNodeWithDescendants).childNodes)
+      cleanNodesInList((node as any).getElementsByTagName('*'));


Why not use childNodes? How are these two options here different?

@brianmhunt the latter is recursive (grandchildren nodes). I changed it to childNodes under the assumption it'd be the same, but it broke the unit tests.

Ah I see. That’s unfortunate since childNodes is much faster .... but evidently incorrect 😀

brianmhunt · 2019-03-16T01:45:33Z

packages/utils/src/dom/virtualElements.ts

+import options from '../options';
+
+// @ts-ignore
+import _let from '../../../tko.binding.core/src/let';


I didn’t see where this was used. I’m sure it is, I just didn’t see where.

brianmhunt · 2019-03-16T01:48:51Z

packages/utils/src/symbol.ts

+
+export const useSymbols = typeof Symbol === 'function';
+
+export function createSymbolOrString(identifier: any) {


I think identifier has to be a string

Can we just use ES2015 Symbols directly and polyfill?

Yes definitely. That’s been on my todo forever

Sebazzz · 2019-03-17T18:07:16Z

packages/utils/src/array.ts

+export const {isArray} = Array;
+
+// export function arrayForEach<T, TThis>(array: T[], action: (this: TThis, item: T, index: number, array: T[]) => void, thisArg: TThis): void;
+export function arrayForEach<T>(array: T[], action: (item: T, index: number, array: T[]) => void, thisArg?: any) {


I see a lot of the same callbacks. It would be nice for DRY-ness to make a seperate type for this (even if only used internally):

type ArrayUtilCallback<T, TResult=void> = (item: T, index: number, array: T[]) => TResult;

@Sebazzz Where should these sorts of types be defined?

Since it is specialized to the utility functions in this file, I'd say in this file at the top.

Got it.

Related question: In the case of JSX, we convert / accept a number of types in anyToNode, with appropriate delegation by a switch e.g. https://github.com/knockout/tko/blob/master/packages/utils.jsx/src/JsxObserver.js#L233

I was thinking of rewriting that switch in TS with something like the following (with most specific arguments to most general):

anyToNode(value: Node) : Node { ... } anyToNode(value: null|undefined|Error|Symbol) : Node { ... } anyToNode(value: object) : Node { ... } anyToNode(value: function) : Node { ... } anyToNode(value: string) : Node { ... } anyToNode(value: boolean|number|bigint) : Node { ... }

If I understand it correctly, Typescript essentially is going to compile this down to a switch anyway. Is there a better way to write this, or is this a sound approach? (Even this fairly naive TS version looks much better to me, certainly, than the JS function with a switch).

Also, the anyToNode returns one of HTMComment, HTMLElement, or Text - what's the right return type? The more general Node? Or the most specific in each case? Or a combination with e.g. HTMLElement|HTMLComment|Text?

I'm not familiar enough with the compiler-side of things regarding JSX, perhaps @mbest knows?
As I understand it, Typescript does not support overloading in the sense that compiler figures out the checks to make, and the handbook does not express it explicitly.

Regarding the last: Yes, a union is the way to go since you know what types might be returned.

Maybe a stupid question, why do we have these in the first place if TKO doesn't target IE8?

@caseyWebb The anyToNode function needs to handle these parameters because it's taking user-based input and turning it into JSX; that input may be in the form of a JSX-like object ({ elementName: string, attributes: object, children: array }) or text, or something else that we nevertheless want to handle gracefully such as an observable or promise.

In order for the compiler to pick the correct typecheck, it follows a similar process to the underlying JavaScript. It looks at the overload list, and proceeding with the first overload attempts to call the function with the provided parameters. If it finds a match, it picks this overload as the correct overload. For this reason, it’s customary to order overloads from most specific to least specific.

^ not sure if that would affect the order you've currently got them in.

@avickers Turns out that section of the docs is only about function type descriptions; Typescript doesn't actually do the overloading implementation. microsoft/TypeScript#5658 . It should. But it doesn't.

That wouldn't be easy. Only for the most trivial cases overloading could be implemented automatically be the Typescript compiler.

Sebazzz · 2019-03-17T18:08:32Z

packages/utils/src/async.ts

+import { safeSetTimeout } from './error';
+
+// tslint:disable-next-line:ban-types
+export function throttle(callback: Function, timeout: number) {


Need to check whether we can make this more specific than Function?

It looks like lodash has a generic Function as well: https://github.com/lodash/lodash/blob/master/throttle.js (though I'm not sure the jsdoc is used for the type checking, I noted that the throttle type at DefinitelyTyped just re-exports)

Sebazzz · 2019-03-17T18:09:35Z

packages/utils/src/css.ts

+    });
+  } else if (typeof (node.className as any).baseVal === 'string') {
+        // SVG tag .classNames is an SVGAnimatedString instance
+    toggleObjectClassPropertyString(node.className, 'baseVal', classNames, shouldHaveClass);


IE < 10 isn't a target anymore right? Perhaps redirect directly to classlist instead of attempting to feature-detect and workaround?

Yeah, there's a ton of pruning we could do for non-old-IE, including this.

brianmhunt · 2019-03-17T19:32:54Z

I'm looking into converting @tko/observable and I'm not sure the best way to add types to an Observable, but there's a lot of questions about things that aren't in the handbook. Here's what I've got for the JS:

export function observable (initialValue) {
  function Observable () {
    if (arguments.length > 0) {
            // Write
            // Ignore writes if the value hasn't changed
      if (Observable.isDifferent(Observable[LATEST_VALUE], arguments[0])) {
        Observable.valueWillMutate()
        Observable[LATEST_VALUE] = arguments[0]
        Observable.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, { value: undefined })

  Observable[LATEST_VALUE] = initialValue

  subscribable.fn.init(Observable)

    // Inherit from 'observable'
  Object.setPrototypeOf(Observable, observable.fn)

  if (options.deferUpdates) {
    deferUpdates(Observable)
  }

  return Observable
}

On the TS side:

type Observable = (...args: any[]) => void

export function observable (initialValue: any) : Observable {
  function Observable (...args: any[]) : Observable {
    if (args.length > 0) {
      if (Observable.isDifferent(Observable[LATEST_VALUE], arguments[0])) { // Property 'isDifferent' does not exist on type '(...args: any[]) => Observable'
        Observable.valueWillMutate()
        Observable[LATEST_VALUE] = arguments[0]
        Observable.valueHasMutated()
      }
      return this // this' implicitly has type 'any' because it does not have a type annotation.ts(2683)
    } 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, { value: undefined })

  Observable[LATEST_VALUE] = initialValue

  subscribable.fn.init(Observable)

    // Inherit from 'observable'
  Object.setPrototypeOf(Observable, observable.fn)

  if (options.deferUpdates) {
    deferUpdates(Observable)
  }

  return Observable
}

These issues seem to mostly stem from the observable function returning an Observable function / instance. It isn't apparent how Typescript is set up to deal with function() { this }, leaving me with a bunch of questions to bridge the gaping chasm that is my TS knowledge. Any thoughts and assistance is most welcome and appreciated.

Sebazzz · 2019-03-17T20:18:23Z

Are you looking for this?

Any reason why Observable is not generic?

brianmhunt · 2019-03-18T02:06:01Z

Any reason why Observable is not generic?

Just because I don't yet know what generic means :)

Sebazzz · 2019-03-18T06:13:57Z

Any reason why Observable is not generic?

Just because I don't yet know what generic means :)

Ah, right, excuse me 😉

So currently any value can be set into the Observable, but if it were an Observable<T> with accepting input and output values of type T instead of any you get a strongly typed observable: If you create a Observable<string> you can't put a Person in it accidently.

brianmhunt · 2019-03-18T12:13:05Z

Any reason why Observable is not generic?

Just because I don't yet know what generic means :)

Ah, right, excuse me 😉

So currently any value can be set into the Observable, but if it were an Observable<T> with accepting input and output values of type T instead of any you get a strongly typed observable: If you create a Observable<string> you can't put a Person in it accidently.

That's unbelievably satisfying. 🤣

…riptify

…ervables

mbest · 2019-03-19T17:19:40Z

See knockout/knockout#2458

skewty · 2019-07-13T05:40:07Z

Is the TS code in this MR functional? It's been over 3 months since a comment was made so I guess everyone got busy with life.

brianmhunt · 2019-07-19T16:15:49Z

@skewty This isn't yet functional (as far as I can tell), but in the interim i've written around 50,000 lines of TS, so my skills are almost up to where they probably need to be to deliver on this PR. 👍

Alongside documentation, converting to TS is my ambition for TKO this year.

Sebazzz and others added 3 commits March 14, 2019 21:36

Typescriptification: initial rename and build configuration

7eb7ca5

Initial Typescriptification of tko.utils - pending compilation issues…

dafa1b0

… in rollup

build: Add TypeScript file (.ts) support

9e41c3c

brianmhunt mentioned this pull request Mar 15, 2019

Typescript support #6

Open

caseyWebb added 3 commits March 15, 2019 19:49

build(tests): Fix Karma w/ TypeScript

7ff758a

build: Don't automatically build on npm/yarn install'

85e25dd

wip

e503483

brianmhunt reviewed Mar 16, 2019

View reviewed changes

Add docs to .gitignore

8369396

Sebazzz reviewed Mar 17, 2019

View reviewed changes

brianmhunt and others added 8 commits March 18, 2019 08:16

@tko/observable) begin typescriptification

0778073

Merge branch 'typescriptify' of github.com:brianmhunt/tko into typesc…

1eb7a8b

…riptify

Update Backers

a39158c

tko.observable) Add more Typescriptification to subscribables and obs…

229bd81

…ervables

configure tslint to use standard config

533164f

wip

2096642

wip

fa7dd07

refactor

bed3ca1

caseyWebb closed this Dec 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TypeScript-ify #92

TypeScript-ify #92

caseyWebb commented Mar 15, 2019

mbest commented Mar 15, 2019

brianmhunt commented Mar 15, 2019

Sebazzz commented Mar 15, 2019 via email •

edited

Loading

brianmhunt commented Mar 15, 2019

Sebazzz commented Mar 15, 2019

brianmhunt Mar 16, 2019

caseyWebb Mar 17, 2019

brianmhunt Mar 17, 2019

brianmhunt Mar 16, 2019

brianmhunt Mar 16, 2019

caseyWebb Mar 17, 2019

brianmhunt Mar 17, 2019

Sebazzz Mar 17, 2019

brianmhunt Mar 17, 2019

Sebazzz Mar 17, 2019

brianmhunt Mar 17, 2019 •

edited

Loading

Sebazzz Mar 17, 2019

caseyWebb Mar 18, 2019

brianmhunt Mar 18, 2019 •

edited

Loading

avickers Mar 19, 2019

brianmhunt Mar 19, 2019

Sebazzz Mar 21, 2019 •

edited

Loading

Sebazzz Mar 17, 2019

brianmhunt Mar 17, 2019

Sebazzz Mar 17, 2019

brianmhunt Mar 17, 2019

brianmhunt commented Mar 17, 2019

Sebazzz commented Mar 17, 2019

brianmhunt commented Mar 18, 2019

Sebazzz commented Mar 18, 2019

brianmhunt commented Mar 18, 2019

mbest commented Mar 19, 2019

skewty commented Jul 13, 2019

brianmhunt commented Jul 19, 2019


		export const useSymbols = typeof Symbol === 'function';

		export function createSymbolOrString(identifier: any) {

TypeScript-ify #92

TypeScript-ify #92

Conversation

caseyWebb commented Mar 15, 2019

TODO

mbest commented Mar 15, 2019

brianmhunt commented Mar 15, 2019

Sebazzz commented Mar 15, 2019 via email • edited Loading

brianmhunt commented Mar 15, 2019

Sebazzz commented Mar 15, 2019

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 Mar 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianmhunt Mar 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sebazzz Mar 21, 2019 • edited Loading

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 commented Mar 17, 2019

Sebazzz commented Mar 17, 2019

brianmhunt commented Mar 18, 2019

Sebazzz commented Mar 18, 2019

brianmhunt commented Mar 18, 2019

mbest commented Mar 19, 2019

skewty commented Jul 13, 2019

brianmhunt commented Jul 19, 2019

Sebazzz commented Mar 15, 2019 via email •

edited

Loading

brianmhunt Mar 17, 2019 •

edited

Loading

brianmhunt Mar 18, 2019 •

edited

Loading

Sebazzz Mar 21, 2019 •

edited

Loading