Skip to content

refactor: service module to TS + ESM (v2)#1166

Merged
06kellyjac merged 83 commits intofinos:mainfrom
jescalada:service-ts-refactor-redone
Oct 17, 2025
Merged

refactor: service module to TS + ESM (v2)#1166
06kellyjac merged 83 commits intofinos:mainfrom
jescalada:service-ts-refactor-redone

Conversation

@jescalada
Copy link
Contributor

@jescalada jescalada commented Aug 24, 2025

resolves #1062
resolves #1245

Note that the changes in the package-lock.json are due to adding the nodemailer types. nodemailer doesn't seem to be used outside of the /src/service/emailSender.ts file which is currently unused, however it will likely be used eventually for solving #1121 (so we might as well keep it unless there's a better email library).

Removed nodemailer and nodemailer/types to reduce package bloat and prevent incompatible license problems. We can always use another email library to implement #1121 later on.

@netlify
Copy link

netlify bot commented Aug 24, 2025

Deploy Preview for endearing-brigadeiros-63f9d0 canceled.

Name Link
🔨 Latest commit 9bc41e2
🔍 Latest deploy log https://app.netlify.com/projects/endearing-brigadeiros-63f9d0/deploys/68f218fee539b8000854acf2

@jescalada jescalada linked an issue Aug 25, 2025 that may be closed by this pull request
3 tasks
@codecov
Copy link

codecov bot commented Aug 27, 2025

Codecov Report

❌ Patch coverage is 73.97770% with 140 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.39%. Comparing base (0b64e7f) to head (9bc41e2).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/service/routes/push.ts 64.00% 29 Missing and 7 partials ⚠️
src/service/passport/oidc.ts 67.24% 17 Missing and 2 partials ⚠️
src/service/passport/ldaphelper.ts 27.27% 16 Missing ⚠️
src/service/passport/jwtAuthHandler.ts 65.85% 9 Missing and 5 partials ⚠️
src/service/routes/auth.ts 73.80% 10 Missing and 1 partial ⚠️
src/service/routes/repo.ts 64.51% 3 Missing and 8 partials ⚠️
src/service/passport/jwtUtils.ts 82.92% 5 Missing and 2 partials ⚠️
src/service/index.ts 91.30% 2 Missing and 2 partials ⚠️
src/service/passport/activeDirectory.ts 75.00% 2 Missing and 2 partials ⚠️
src/service/routes/config.ts 66.66% 4 Missing ⚠️
... and 8 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1166      +/-   ##
==========================================
- Coverage   83.84%   83.39%   -0.45%     
==========================================
  Files          68       70       +2     
  Lines        2946     3006      +60     
  Branches      380      501     +121     
==========================================
+ Hits         2470     2507      +37     
+ Misses        409      396      -13     
- Partials       67      103      +36     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jescalada jescalada marked this pull request as ready for review August 27, 2025 09:27
@jescalada jescalada requested a review from a team August 27, 2025 09:31
Copy link
Contributor

@kriswest kriswest left a comment

Choose a reason for hiding this comment

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

Looks pretty good, a few small niggles to resolve

jescalada and others added 6 commits October 10, 2025 12:51
Co-authored-by: Kris West <kristopher.west@natwest.com>
Signed-off-by: Juan Escalada <97265671+jescalada@users.noreply.github.com>
fix: return on /create-user
fix: cast res,data to Boolean in ldaHelper
@jescalada
Copy link
Contributor Author

@kriswest Left a comment regarding the gitReviewerAccount which I removed completely from the endpoint as it was unused. The rest is ready for a final look!

@jescalada jescalada requested a review from kriswest October 10, 2025 11:11
@jescalada
Copy link
Contributor Author

As for the reduced test coverage, this is mostly due to all the extra conditionals for type checking I added (AKA "partial" coverage). I'd like to wrap up the Vitest conversion PR to see the new coverage, then assess whether we need to improve our tests once the full TS refactor is over.

@kriswest
Copy link
Contributor

@jescalada Should we delete test/fixtures/test-package/package-lock.json
and add it to the .gitignore?

@kriswest
Copy link
Contributor

Update typing for startServer in packages/git-proxy-cli/test/testCliUtils.ts before closing out

@06kellyjac
Copy link
Contributor

If this is rebased and the above is addressed, plus a squash for good measure, I think we can merge

@jescalada
Copy link
Contributor Author

@06kellyjac Unfortunately rebasing proved to be a pain - I ended up merging from main instead...

@kriswest's comments are solved now. It seems that the package-lock.json in the test fixtures is generated by one of the tests, I've added it to the .gitignore and deleted it too.

Copy link
Contributor

@kriswest kriswest left a comment

Choose a reason for hiding this comment

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

LGTM

@kriswest
Copy link
Contributor

kriswest commented Oct 17, 2025

mergeable - don't forget to squash!

@06kellyjac 06kellyjac merged commit 13ee734 into finos:main Oct 17, 2025
14 checks passed
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.

Don't require a github username to be set to file a review Refactor service module to TS [Migration] Convert /src/service files to TS and ESM (API)

4 participants