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

♻️ Maintenance: Improve TypeScript support #1279

Merged
merged 6 commits into from
Mar 22, 2024

Conversation

segersniels
Copy link
Collaborator

@segersniels segersniels commented Mar 20, 2024

Description

After reading reports from people trying their hand on converting files to TS, I noticed the setup wasn't fully allowing conversion without causing a big waterfall of required changes. While it was already possible to import Flow files into TS, it apparently wasn't yet possible to import TS files into Flow without the need for a lot of changes. This PR basically attempts to fix whatever was needed so proper TS work can start again 🚀

Changed

  • Adjusted babel to properly handle all .ts files and not just the ones in ./src
  • Adjusted tsconfig.json to properly output ESM so it can be used alongside other ESM modules like meow and execa
  • withClient.ts has been converted to TS

Fixed

  • Jest now properly looks for .ts files and includes them in the test run
  • cli.spec.ts has not been running since the original TS introduction and is now running again

Issue: #1110 (comment)

Tests

  • All tests passed.

Copy link

codecov bot commented Mar 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.43%. Comparing base (150a350) to head (eb95a1e).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1279      +/-   ##
==========================================
+ Coverage   91.72%   92.43%   +0.71%     
==========================================
  Files          28       29       +1     
  Lines         278      291      +13     
  Branches       66       66              
==========================================
+ Hits          255      269      +14     
+ Misses         22       21       -1     
  Partials        1        1              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@segersniels segersniels self-assigned this Mar 20, 2024
@segersniels segersniels marked this pull request as draft March 20, 2024 23:11
@segersniels
Copy link
Collaborator Author

segersniels commented Mar 20, 2024

@carloscuesta I know the TS version of cli.spec.js hasn't been running since the refactor, but it seems that when I revert it back to the state it was before the refactor it doesn't even run. Did that test ever run? It seems like a bunch of stubs are missing to actually make everything work. Not entirely familiar with jest as I usually work with mocha & chai so I might be missing something obvious.

@carloscuesta
Copy link
Owner

@carloscuesta I know the TS version of cli.spec.js hasn't been running since the refactor, but it seems that when I revert it back to the state it was before the refactor it doesn't even run. Did that test ever run? It seems like a bunch of stubs are missing to actually make everything work. Not entirely familiar with jest as I usually work with mocha & chai so I might be missing something obvious.

Hey!

It's probably because of this:

gitmoji-cli/package.json

Lines 97 to 99 in 150a350

"testMatch": [
"**/*.(spec).(js)"
],

This is the pattern that jest uses to find which spec files to run, since the cli.spec was moved to TS we need to adjust this to:

"testMatch": [
      "**/*.(spec).(js)",
      "**/*.(spec).(ts)"
    ],

So jest can find the .spec.ts files 👍🏼

@segersniels
Copy link
Collaborator Author

segersniels commented Mar 21, 2024

@carloscuesta I know the TS version of cli.spec.js hasn't been running since the refactor, but it seems that when I revert it back to the state it was before the refactor it doesn't even run. Did that test ever run? It seems like a bunch of stubs are missing to actually make everything work. Not entirely familiar with jest as I usually work with mocha & chai so I might be missing something obvious.

Hey!

It's probably because of this:

gitmoji-cli/package.json

Lines 97 to 99 in 150a350

"testMatch": [
"**/*.(spec).(js)"
],

This is the pattern that jest uses to find which spec files to run, since the cli.spec was moved to TS we need to adjust this to:

"testMatch": [
      "**/*.(spec).(js)",
      "**/*.(spec).(ts)"
    ],

So jest can find the .spec.ts files 👍🏼

That was the original issue indeed, which we missed back when we refactored a couple of months ago (and has been changed in this PR). Test has been changed back to cli.spec.js in this PR and is running again, but failing on missing stubs. So I was wondering if it ever ran correctly or something broke with the jest mocks. You probably know more about jest than me

@carloscuesta
Copy link
Owner

carloscuesta commented Mar 21, 2024

Yes, the test was running and passing successfully from the beginning of it's inception you can check it for example by going to the commit previous to the introduction of TS support and running the test:

git checkout c0b0ab8
yarn install
yarn test

Actually the issue is that the mock was not properly implemented on that PR https://github.com/carloscuesta/gitmoji-cli/pull/1125/files#r1229243943

CleanShot 2024-03-21 at 22 08 37@2x

@segersniels
Copy link
Collaborator Author

segersniels commented Mar 21, 2024

Yes, the test was running and passing successfully from the beginning of it's inception you can check it for example by going to the commit previous to the introduction of TS support and running the test:

git checkout c0b0ab8
yarn install
yarn test

Actually the issue is that the mock was not properly implemented on that PR https://github.com/carloscuesta/gitmoji-cli/pull/1125/files#r1229243943

CleanShot 2024-03-21 at 22 08 37@2x

I reverted all changes for that file in this PR but it's still not running it seems. Which is weird to me because it should be identical to what it was before the refactor.

package.json Outdated Show resolved Hide resolved
@carloscuesta
Copy link
Owner

carloscuesta commented Mar 21, 2024

I reverted all changes for that file in this PR but it's still not running it seems. Which is weird to me because it should be identical to what it was before the refactor.

It's tricky to exactly pin point what's the source of the issue now, as the spec file was not running since the refactor and several dependency updates were merged. What we certainly know is that before that specific commit it was passing as of this #1279 (comment)

So if you want to understand where the issue was exactly introduced (after reverting the TS PR) what I would suggest is doing a bisect https://git-scm.com/docs/git-bisect setting this hash as good c0b0ab8 and the latest commit in master as bad

@segersniels
Copy link
Collaborator Author

segersniels commented Mar 21, 2024

I reverted all changes for that file in this PR but it's still not running it seems. Which is weird to me because it should be identical to what it was before the refactor.

It's tricky to exactly pin point what's the source of the issue now, as the spec file was not running since the refactor and several dependency updates were merged. What we certainly know is that before that specific commit it was passing as of this #1279 (comment)

So if you want to understand where the issue was exactly introduced (after reverting the TS PR) what I would suggest is doing a bisect https://git-scm.com/docs/git-bisect setting this hash as good c0b0ab8 and the latest commit in master as bad

My guess is the dynamic imports that we introduced as part of speed improvements. Jest doesn't seem to be handling those very well. I'll have a closer look when I have the time 👍

EDIT: Turns out it was exactly that. Basically the only mock was on ../src/commands which wasn't a valid mock anymore due to the @commands/commit/index.js dynamic imports in cli.ts. After updating the mocks for each individual dynamic import the test is now running again ✅ + changed back to TS to have no regression in TS conversion

@segersniels segersniels marked this pull request as ready for review March 21, 2024 22:26
@segersniels segersniels changed the title ♻️ Maintenance: Further TS support ♻️ Maintenance: Improve TypeScript support Mar 21, 2024
tsconfig.json Show resolved Hide resolved
package.json Show resolved Hide resolved
@segersniels segersniels merged commit 5a44226 into master Mar 22, 2024
3 checks passed
@segersniels segersniels deleted the maintenance/ts-conversion branch March 22, 2024 17:56
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants