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

Migrate frint-cli to TypeScript #400

Merged
merged 9 commits into from
Jan 11, 2018
Merged

Migrate frint-cli to TypeScript #400

merged 9 commits into from
Jan 11, 2018

Conversation

markvincze
Copy link
Contributor

This is a starting point to move frint to TypeScript.
I started with frint-cli, because it's less critical, and it's self-contained. If this looks good and we agree to start moving to TS, then the core frint package would be a good next candidate.

Although frint-cli is really simple and its code itself is not really utilizing TS, the tooling is already valuable. For example these are producing errors at compile time without running any tests, while when using JS, we don't see anything.

image

image

And we get proper autocomplete too out of the box, and we don't have to maintain d.ts files manually:

image

The exciting part is going to be migrating frint, because there we'll have to make interesting decisions about how to design the TS type model.

"test": "cross-env ../../node_modules/.bin/mocha --colors --recursive ./commands/*.spec.js",
"lint": "cross-env ./node_modules/.bin/tslint -p .",
"transpile": "cross-env ./node_modules/.bin/tsc",
"test": "cross-env ../../node_modules/.bin/mocha --require ts-node/register --colors --recursive ./src/**/*.spec.ts",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are multiple ways to run TS unit tests.
We could also simply transpile first and then run mocha on the transpiled JS, but this option to use ts-node seems to be more popular (and this way watch can work properly too).

@@ -3,7 +3,7 @@
"description": "Framework for building front-end apps",
"private": true,
"scripts": {
"bootstrap": "lerna bootstrap",
"bootstrap": "lerna bootstrap --hoist",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The --hoist flag is needed due to microsoft/TypeScript#20945

@markvincze
Copy link
Contributor Author

markvincze commented Jan 9, 2018

The CI build is failing due to linting, I'll check that when I'll have time.
Yep, it was just linting, fixed it.

@codecov
Copy link

codecov bot commented Jan 9, 2018

Codecov Report

Merging #400 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #400   +/-   ##
=======================================
  Coverage   97.39%   97.39%           
=======================================
  Files         101      101           
  Lines        3992     3992           
=======================================
  Hits         3888     3888           
  Misses        104      104

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 071ff5d...b273299. Read the comment docs.

@fahad19
Copy link
Member

fahad19 commented Jan 9, 2018

💖💖💖

Many thanks for leading this TypeScript initiative, @markvincze!!!

(Will check this PR out in more details toorrow)

@discosultan
Copy link
Contributor

merge

@fahad19
Copy link
Member

fahad19 commented Jan 11, 2018

Refs #388

@markvincze markvincze merged commit 9275698 into master Jan 11, 2018
@fahad19 fahad19 mentioned this pull request Feb 14, 2018
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants