jest-community / jest-editor-support

A module for handling editor to jest integration
MIT License
28 stars 21 forks source link

Typescript conversion #32

Closed rossknudsen closed 4 years ago

rossknudsen commented 4 years ago

This is a conversion to Typescript, removing Flow and updating build scripts.

Unit tests should be passing, but I need to manually test. Maybe we can get a pre-release, or is there another simple way to substitute an npm package in my extension?

rossknudsen commented 4 years ago

Hmmm, looks like I need to fix the linting...

rossknudsen commented 4 years ago

Ah looks like the version of node on the CI server is too low to run one of the new dependencies. Can we bump the version?

connectdotz commented 4 years ago

while I like typescript a lot personally, I am not the only contributor in this repo, @stephtr @seanpoulter, are you guys ok to move the repo to typescript?

stephtr commented 4 years ago

while I like typescript a lot personally, I am not the only contributor in this repo, @stephtr @seanpoulter, are you guys ok to move the repo to typescript?

Personally, I definitely prefer working with TypeScript. It would also better fit to vscode-jest.

rossknudsen commented 4 years ago

Huh, somehow the coverage dropped...

rossknudsen commented 4 years ago

while I like typescript a lot personally, I am not the only contributor in this repo, @stephtr @seanpoulter, are you guys ok to move the repo to typescript?

Fair enough, I was going by this comment which is probably old/stale now. I ran across the comment because I wanted to access more of the Jest settings. I guess I went down a bit of a rabbit hole with this one...

seanpoulter commented 4 years ago

Thanks for the huge effort @rossknudsen! 👏

--

@rossknudsen wrote

Unit tests should be passing, but I need to manually test. Maybe we can get a pre-release, or is there another simple way to substitute an npm package in my extension?

Yes, you can substitute npm packages using "linking". Here's the docs for npm and yarn that are probably more clear than what I'd write. ;)

--

@connectdotz wrote:

while I like typescript a lot personally, I am not the only contributor in this repo, @stephtr @seanpoulter, are you guys ok to move the repo to typescript?

I've got two answers for this: one with best interest of the project in mind, and the other from my own perspective.

  1. With the best interest in the project in mind, I'd say yes, switching to TypeScript makes sense because folks are more familiar with TypeScript than Flow. I don't think any of the other reasons I've considered sway me one way or the other.
    Pros
    • More type information will help folks use the library in lieu of documentation
    • We can leverage more of the types exported from the facebook/jest repo
    • TypeScript has more adoption than Flow [1] [2]
    • TypeScript may have better IDE integration than Flow
    • TypeScript might improve our code quality
    Cons
    • TypeScript is still a barrier to entry over "regular" "JavaScript" -- not everyone will keep up with the ES20xx language features
    • Spending time on test case coverage and code review may be a better time investment [3]
  2. With my own interest in mind, I see this is a PR that's too big for me to approach so I'm biased towards keeping the status quo. While I think this is adding a lot of value, we may have missed some of the conversations we need like:
    • Do the maintainers have the capacity to review the PR? (I don't)
    • Can we make the switch from Flow to TypeScript in incremental, easy to review PRs?
    • Do we need to discussion what TS config we'll use, and is it familiar? (As an arbitrary example, this is the first time I've seen optional chaining in a repo and sometimes they feel icky, like in tests).
    • What test cases do we need to have confidence in this, especially if it's too big to review?
    • (edited to add this): Are we OK to migrate AND refactor at the same time? For example, can we convert the Flow Maybe to the TypeScript optional?

[1]: https://2019.stateofjs.com/javascript-flavors/typescript/ [2]: https://2019.stateofjs.com/javascript-flavors/other-tools/ [3]: https://medium.com/javascript-scene/the-typescript-tax-132ff4cb175b

rossknudsen commented 4 years ago

@seanpoulter I appreciate the time taken to write your thoughts regarding this PR. I reviewed the changed files again and it is substantial. My original intent was to avoid a massive delta as it has potential to introduce regressions etc and part of the reason why people wouldn't want to convert in the first place. Also 100% appreciate your limited time in reviewing this.

In light of all this, I stepped back and thought about whether its possible to take an incremental approach to conversion. So I looked at the recommendations for conversion from Microsoft and cherry-picked some changes from this branch into a new branch. If you like this approach I can open another PR for review. I think I've kept everything operational while converting just one file to Typescript.

connectdotz commented 4 years ago

In light of all this, I stepped back and thought about whether its possible to take an incremental approach to conversion... If you like this approach I can open another PR for review. I think I've kept everything operational while converting just one file to Typescript.

I like that! 👍

connectdotz commented 4 years ago

superseded by #33