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

Upgrade to Typescript #50

Merged
merged 9 commits into from
Dec 12, 2019
Merged

Upgrade to Typescript #50

merged 9 commits into from
Dec 12, 2019

Conversation

Jack-Barry
Copy link

What's the problem this PR addresses?

Removes Flow and switches the repo over to TypeScript, as per discussion in #17

How did you fix it?

  1. Used flow-to-ts to convert all src files
  2. Used flow-to-ts to convert applicable test files
  3. Fixed type errors or threw in a // @ts-ignore where it wasn't worth the battle for now
  4. Ensured tests were still passing with some minor adjustments
  5. Ensured coverage reporting is functioning properly
  6. Updated tooling scripts to use TypeScript

Some of these commits overlap with #49 PR but I'm keeping this one separate for tracking purposes since it's a different issue being addressed more massive overhaul of the tooling. The other PR is ready to go and has passed all tests whereas this one needs to be vetted via CI tooling and probably requires further review.

@larixer
Copy link
Member

larixer commented Dec 12, 2019

@Jack-Barry Could you merge with updated upstream master please?

@Jack-Barry
Copy link
Author

One thing to note here is that the code coverage percentage currently reported (95%) will drop significantly.

Given the brittle nature of some of the tests this doesn't surprise me, I am inclined to believe the new value reported is more accurate. If there's something I missed in updating the coverage reporting in the shift over to TS just let me know, thanks!

@larixer
Copy link
Member

larixer commented Dec 12, 2019

@Jack-Barry Good stuff, many thanks!

@larixer larixer merged commit d54cb02 into sysgears:master Dec 12, 2019
@JamesMcMahon
Copy link
Contributor

Thanks for biting this one off.

@Jack-Barry
Copy link
Author

Before

BeforeTS

After

AfterTS

Ahhh, that's better 😊

@larixer
Copy link
Member

larixer commented Dec 12, 2019

@Jack-Barry I fully agree! :)

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.

3 participants