Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial Typescriptification of tko.utils #62

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Sebazzz
Copy link

@Sebazzz Sebazzz commented Apr 25, 2018

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 Up for grabs? #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.

@mbest
Copy link
Member

mbest commented Apr 25, 2018

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.

@Sebazzz
Copy link
Author

Sebazzz commented Apr 26, 2018

You're right. I will do it evening.

@caseyWebb
Copy link
Contributor

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

@Sebazzz
Copy link
Author

Sebazzz commented Apr 26, 2018

@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.
5154c99

@Sebazzz
Copy link
Author

Sebazzz commented Apr 26, 2018

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

@caseyWebb
Copy link
Contributor

caseyWebb commented Apr 26, 2018

[x-ref]: Profiscience/knockout-contrib#25 (comment)

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.

@brianmhunt
Copy link
Member

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!

@caseyWebb
Copy link
Contributor

caseyWebb commented Apr 27, 2018

  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.

@maskmaster
Copy link

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

@Sebazzz
Copy link
Author

Sebazzz commented Apr 27, 2018

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.

@brianmhunt
Copy link
Member

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.

@Sebazzz
Copy link
Author

Sebazzz commented Apr 29, 2018

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.

@brianmhunt
Copy link
Member

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.

@Sebazzz
Copy link
Author

Sebazzz commented Apr 30, 2018

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.

@caseyWebb
Copy link
Contributor

caseyWebb commented Apr 30, 2018

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

@Sebazzz
Copy link
Author

Sebazzz commented Apr 30, 2018

@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.

@brianmhunt
Copy link
Member

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.

@caseyWebb caseyWebb mentioned this pull request Mar 15, 2019
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants