Skip to content

refactor(ts): refactor proxy to TS #960

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

Merged
merged 51 commits into from
Apr 7, 2025

Conversation

jescalada
Copy link
Contributor

@jescalada jescalada commented Mar 28, 2025

Fixes #965.

This PR refactors most of the proxy logic to TS. It builds on the db and config refactor PRs.

The tests are passing, but they do not cover the push/pull processor functions (because the logic is stubbed). There's a fix for this in #805, which might be worth looking at!

fabiovincenzi and others added 30 commits February 17, 2025 16:12
Copy link

codecov bot commented Mar 28, 2025

Codecov Report

Attention: Patch coverage is 72.80514% with 127 lines in your changes missing coverage. Please review.

Project coverage is 50.08%. Comparing base (9046dd4) to head (5f3de0a).
Report is 52 commits behind head on main.

Files with missing lines Patch % Lines
src/db/file/repo.ts 50.00% 23 Missing ⚠️
src/db/file/pushes.ts 47.05% 18 Missing ⚠️
src/proxy/processors/push-action/parsePush.ts 48.57% 18 Missing ⚠️
src/db/index.ts 42.85% 11 Missing and 5 partials ⚠️
src/proxy/actions/Action.ts 64.10% 14 Missing ⚠️
...roxy/processors/push-action/checkCommitMessages.ts 54.54% 5 Missing ⚠️
src/plugin.ts 0.00% 4 Missing ⚠️
src/proxy/processors/pre-processor/parseAction.ts 50.00% 4 Missing ⚠️
src/proxy/routes/index.ts 66.66% 4 Missing ⚠️
src/db/file/users.ts 80.00% 3 Missing ⚠️
... and 9 more
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #960       +/-   ##
===========================================
- Coverage   62.54%   50.08%   -12.46%     
===========================================
  Files          50       48        -2     
  Lines        1842     1711      -131     
  Branches        0      174      +174     
===========================================
- Hits         1152      857      -295     
- Misses        690      834      +144     
- Partials        0       20       +20     

☔ 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 changed the title Refactor proxy to TS refactor(ts): refactor proxy to TS Mar 28, 2025
@jescalada jescalada linked an issue Mar 29, 2025 that may be closed by this pull request
3 tasks
Copy link
Contributor

@coopernetes coopernetes left a comment

Choose a reason for hiding this comment

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

Wonderful PR - thank you @jescalada for picking this up! This has been a thorn in the side of this project for awhile now and very pleased to see typescript finally land. Appreciate this contribution.

I've left a few areas where I think more types can be added as well as a minor dependency change.

@coopernetes
Copy link
Contributor

@JamieSlome we will need to contact an org admin to modify the required PR status check. It is explicitly set to build (18.x, 4.4) and this PR introduces a Node version upgrade to 22. We should probably set the build job name to something generic so that we can upgrade node versions without having to modify GitHub repo settings.

@jescalada
Copy link
Contributor Author

Thanks for the review @coopernetes!

I originally bumped the Node CI to 22 due to some ESM errors, but I managed to fix them. That said, there's a crypto is not defined error when using OIDC login in Node 18. Bumping it to v19 fixes this.

@jescalada jescalada force-pushed the refactor-proxy-module-ts-and-esm branch from 390de82 to 90a8cab Compare March 31, 2025 06:35
@jescalada
Copy link
Contributor Author

Hi @coopernetes, I just fixed up those lines you mentioned, and did some manual testing to make sure the flow still works as expected.

I'm a bit confused about this part on parsePush.ts. What's the reasoning for this, and should we apply this to all the Action files?

-const exec = async (req: any, action: Action): Promise<Action> => {
+async function exec(req: any, action: Action): Promise<Action> {

Thanks!

@coopernetes
Copy link
Contributor

Hi @coopernetes, I just fixed up those lines you mentioned, and did some manual testing to make sure the flow still works as expected.

I'm a bit confused about this part on parsePush.ts. What's the reasoning for this, and should we apply this to all the Action files?

-const exec = async (req: any, action: Action): Promise<Action> => {

+async function exec(req: any, action: Action): Promise<Action> {

Thanks!

That's just a style preference. I'm not against the use of arrow functions so long as we have clear typing on the function arguments & return type.

Copy link
Contributor

@coopernetes coopernetes left a comment

Choose a reason for hiding this comment

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

I will suggest we add an eslint rule to warn on those any usages so we can clean them up over time. Otherwise, LGTM.

The plugin code could use more strict typing but since it's doing dynamic module loading, this is a good way forward for the rest of the proxy server code.

@coopernetes
Copy link
Contributor

@jescalada Can you resolve the conflicts and then this can be merged? thanks!

@coopernetes coopernetes enabled auto-merge April 7, 2025 15:16
auto-merge was automatically disabled April 7, 2025 15:43

Head branch was pushed to by a user without write access

@jescalada
Copy link
Contributor Author

jescalada commented Apr 7, 2025

@coopernetes Thank you again for the review! The merge conflicts required me to refactor one more file. I didn't manually test the changes, but the unit tests seem to pass.

Just to remind you, #805 fixes the proxy tests for more code coverage (quite crucial since the core proxy logic is stubbed!). Maybe we could ping the author to remind them to complete the CLA?

Thanks!

@coopernetes coopernetes merged commit 38791bd into finos:main Apr 7, 2025
14 checks passed
@JamieSlome
Copy link
Member

Great job @jescalada and @coopernetes for the review ❤️

I assume this is at least a minor bump, potentially even major? Let me know and I'll schedule a new release 🚀

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.

Refactor proxy module into TS [Migration] Convert /src/proxy module to TS (core proxy logic)
4 participants