tko icon indicating copy to clipboard operation
tko copied to clipboard

Initial Typescriptification of tko.utils

Open Sebazzz opened this issue 7 years ago • 18 comments
trafficstars

Work in progress

As discussed in #6, initial Typescriptification of the core tko.utils package. This first pull request will set the direction for further Typescriptification, so let's get it right 😄

Notes:

  • Many of the lower-level DOM compatibility layers use a lot of any though I have attempted to minimalize it
  • Added initial tslint config file for Typescript linting
  • As discussed in #61, due to check-out issues on Windows, replaced the node_modules symlink
  • Specs are not Typescriptified (imho not very important)
  • Due to Rollup build issues I didn't run any tests yet.
  • Typescript is configured for ES5 emitting.

There are still some pending issues, which I am currently not being able to work out due to my unfamiliarity with rollup:

  • Rollup always wants to take the index.js of a package, but in this case we have a index.ts. What can be done here? Possible solution: ~rename all index.js to index.ts~ nope that does not work, rollup attempts to parse Typescript now.
  • Currently no (typed) support for the this parameter in many of the array utils due to compilation errors. Typescript doesn't appear to correct method resolution.

Sebazzz avatar Apr 25 '18 20:04 Sebazzz

To make it easier to see the changes to each file, I think it would good to split this into two commits. The first just renames the files and the second has the actual changes.

mbest avatar Apr 25 '18 22:04 mbest

You're right. I will do it evening.

Sebazzz avatar Apr 26 '18 06:04 Sebazzz

Using git mv will preserve the file history, and not show the entire file as diffed (so 2 commits aren't needed).

caseyWebb avatar Apr 26 '18 16:04 caseyWebb

@caseyWebb No, that does unfortunately does not work. If do a git mv, then apply the changes, git still decides it is a delete/add. Same when I commit, then amend. And the same when I commit twice, then squash.

I have committed again, changes are visible in second commit. The Github diff causes the same issues. https://github.com/knockout/tko/pull/62/commits/5154c99cec9b18a069515b541f775e52cb6da34e

Sebazzz avatar Apr 26 '18 18:04 Sebazzz

It appears the rollup Typescript plugin is not really being maintained, and having multiple issues: https://github.com/rollup/rollup-plugin-typescript/issues/116

Sebazzz avatar Apr 26 '18 18:04 Sebazzz

[x-ref]: https://github.com/Profiscience/knockout-contrib/issues/25#issuecomment-376030856

Just want to get clarification, are we going for TS or JS source for TKO? I'd started playing around with wiring up type-checking unobtrusively (only a handful of files), but I'm all for actually using TS the way it's intended to be used. If that is the case, I can start bringing in bits and pieces of knockout-contrib (see issue above).

@Sebazzz I didn't realize you still had to commit the move as a separate commit. That is... not optimal. Silly git.

caseyWebb avatar Apr 26 '18 18:04 caseyWebb

This is great, thanks @Sebazzz . There are a couple things I'd like to understand.

  1. Style — there's a lot of munging of the changelog because of extra semicolons and such; I'd like to stick with StandardJS if possible. Is there a TS equivalent? I googled but found nothing.

  2. Typescript — I'd like to avoid Typescript as a hard dependency, and prefer to have the core written in ES6 with Typescript definitions "added on", so to speak. The reason is that, basically, I don't trust Typescript to be around forever or to go the direction we need it to — I've only used it for a few hours and already found a couple serious bugs, one of which is a WONTFIX. I have trust issues 😄 , but I'm wary of the work needed to back out of TS if it becomes necessary —reminiscing on having had to spend a huge amount of time converting Coffeescript projects, I enjoy the freedom of having ES6 as the lowest common denominator! Is it possible to write the Typescript definitions in separate files, keeping the ES6 to itself? In other words - does it have to be tightly coupled?

  3. Core competencies — I/we may not be able to support it as well, at least not at the beginning. TS isn't something I've used at all, and we don't want half the codebase in TS and the other half in ES6. It's a pretty big conversion, and we'd need to know that we won't get 2/3rds of the way through only to lose the capacity.

None of these are blockers, but I feel it important to voice these concerns to make sure I understand the implications. Typescript might be a great foundation for the TKO/KO future, but I'd at least like to know about the options before we commit to this fairly hefty change.

Thanks @Sebazzz — really appreciate your effort here, and hope to have these issues clarified soon!

brianmhunt avatar Apr 27 '18 11:04 brianmhunt

  1. Prettier. Let's use prettier.

  2. It doesn't have to be, but the type-checking will not be as good. You can write external definitions and still get some type checking, but it's not idiomatic and more of a hack with the legacy TS ecosystem.

  3. If strict is not enabled in tsconfig.json, you can change the file extension of a js file to ts, and it will work and compile file as long as there aren't actual issues discovered. Note though, you want strict. It will save you from yourself.

Been down the coffeescript road before as well and regretted it immensely. That said, TS is a different beast, because JS is TS (without types, which without strict are optional), making the transition almost seamless. In my experience, it's quick to pick up as well, especially if you have experience with C#.

Personally, I really think the benefits outweigh any potential negatives. I didn't really "get it" until I used it with VS Code and remembered how nice actual autocomplete and go-to-implementation were. It has significantly increased the robustness of our codebase since we began using it at work.

caseyWebb avatar Apr 27 '18 13:04 caseyWebb

Also, typescript is transpiling to standard JS. The result usually looks good with no strange additions in the generated js code.

maskmaster avatar Apr 27 '18 14:04 maskmaster

Please note that Typescript found already two actual bugs in the codebase. I understand the doubt, but the benefits of type safety are already showing. And this is just one small component that has been transformed.

The basic Typescript escape plan is:

  1. Compile to current level ES
  2. Use that as a code base

Typescript continues to aim to be compatible with ES language specs, and the features which aren't are marked expirimental and needs to be manually enabled (and shouldn't you use anyway in a distributed library).

I've only used it for a few hours and already found a couple serious bugs, one of which is a WONTFIX.

(your link is broken, you appear to reference this issue) That appears to be due to downlevel compilation concerns though.

Sebazzz avatar Apr 27 '18 14:04 Sebazzz

Great feedback, thanks all. It definitely sounds like the benefits are there, and there's a lot of momentum behind TS so I can see how it's distinguished from Coffeescript.

I've never used Prettier - but it seems to solve same pain as StandardJS. I'd prefer --no-semi, unless there are objections.

@knockout/core — Any thoughts on TS & the tools?

Unless there are major objections & concerns, or information I don't know, I'm inclined to agree to moving to strict TS.

brianmhunt avatar Apr 27 '18 16:04 brianmhunt

I prefer explicit semicolons over automatic semicolon insertion. The reason is that the latter may be ambiguous (another example), so in some situations you are required to use semicolons. In my opinion it is better to be consistent and then use them everywhere and I believe it is good practice in general.

And this is personal taste: I'm able to parse code faster with semicolons because you can quickly scan where each statements ends. But perhaps this is old fashioned.

Sebazzz avatar Apr 29 '18 09:04 Sebazzz

The main argument to not include semis at this point is because the diffs will have a poor signal-to-noise ratio i.e. any semantic differences will be indistinguishable from semicolon injection.

We can revisit the semicolon after, but for the JS→TS PRs we should keep the diffs crisp.

brianmhunt avatar Apr 30 '18 12:04 brianmhunt

I understand that. You would still need to configure prettier to use the current coding standard as much as possible, otherwise the noise in the diff will be too high, it is good to be aware of that.

Sebazzz avatar Apr 30 '18 17:04 Sebazzz

Prettier is less a linter and more a formatter. Like StandardJS, it's opinionated and has minimal configuration options. While ESLint/TSLint can do some formatting, it is the primary goal of prettier, so it's a lot more powerful. If you've ever worked in go, it's akin to gofmt. In fact, we may find we want to use it with ESLint/TSLint to catch issues related to quality and not style.

I think it may be best to format the entire codebase in a separate PR to get all of the noise out of the way up front. (That said, I still don't like semis...)

P.S. @Sebazzz Let me know if/what help you need with the Rollup side of this

caseyWebb avatar Apr 30 '18 17:04 caseyWebb

@caseyWebb Thanks. I'm unable to get actual compilation of Typescript working with the current configuration. So I could definitely use some help there :) It appears Rollup is currently configured to take an index.js file, while for tko.utils a index.ts file is now used.

Sebazzz avatar Apr 30 '18 18:04 Sebazzz

Just a quick note - I've investigated a bit and am 100% on board with Typescript.

I'm pulling in the PR #50 to bring TKO up to speed with Knockout 3.5/6, and then this is on the list.

brianmhunt avatar May 25 '18 12:05 brianmhunt