Skip to content

refactor: Add TypeScript support#1142

Merged
06kellyjac merged 18 commits intofinos:mainfrom
fabiovincenzi:typescript-setup
Oct 22, 2025
Merged

refactor: Add TypeScript support#1142
06kellyjac merged 18 commits intofinos:mainfrom
fabiovincenzi:typescript-setup

Conversation

@fabiovincenzi
Copy link
Contributor

Overview
This PR introduces TypeScript to git-proxy and refactors relevant code to support it.

Changelog

  • Added TypeScript configuration (tsconfig.json) with essential settings for strict type checking, ES6 compatibility, and JSX support.
  • Added typescript and ts-node to manage TypeScript code compilation and execution.
  • Updated the package.json to include TypeScript dependencies.
  • Converted the main entry file from JavaScript (index.js) to TypeScript (index.ts).
  • Modified the start script to use ts-node for running TypeScript files.

Thanks to @jescalada for the work on fixing CI issues on G-Research#31, which helped guide the necessary changes in this PR.

Related issue: #927


Note: Restored from deleted fork

This PR recreates the original PR #929, which was automatically closed due to accidental fork deletion.

** For discussions and reviews:** See the original PR #929

All commits are identical to the original with preserved git history.

@fabiovincenzi fabiovincenzi linked an issue Aug 4, 2025 that may be closed by this pull request
3 tasks
@netlify
Copy link

netlify bot commented Aug 4, 2025

Deploy Preview for endearing-brigadeiros-63f9d0 canceled.

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

@codecov
Copy link

codecov bot commented Aug 5, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.64%. Comparing base (375bd13) to head (b9f9dcf).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1142      +/-   ##
==========================================
- Coverage   83.40%   82.64%   -0.77%     
==========================================
  Files          70       70              
  Lines        3007     3007              
  Branches      501      501              
==========================================
- Hits         2508     2485      -23     
- Misses        396      419      +23     
  Partials      103      103              

☔ 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.

Copy link
Contributor

@jescalada jescalada left a comment

Choose a reason for hiding this comment

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

Reapproving this - it's a lot easier to see the changes now that merge conflicts have been resolved 👍🏼

@coopernetes @06kellyjac I suppose this could go into the first release candidate for v2? Or should we deal with the few remaining TS PRs and get it all in a single release? #960 #1063 #1065

Copy link
Contributor

@06kellyjac 06kellyjac left a comment

Choose a reason for hiding this comment

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

Yes, moving to providing js and types from dist should be done in v2.
https://github.com/yeoman/generator/blob/main/package.json#L23-L37

Also there's several useful things within git-proxy which we expect people to directly import from files e.g. config, which would break & should be exported properly.

@06kellyjac
Copy link
Contributor

considering the majority of commits are just update merges and there's 2 commits with the same message a rebase & cleanup would be ideal

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 15, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: fabiovincenzi / name: Fabio Vincenzi (b9f9dcf)

@jescalada
Copy link
Contributor

Wondering if this is ready to go? 🤔

@fabiovincenzi @06kellyjac

@fabiovincenzi
Copy link
Contributor Author

Hi @06kellyjac, what do you think about the latest changes?

@finos-admin
Copy link
Member

Given that @JamieSlome was covered by the Citi CCLA at the time the commits were made we can ignore the EasyCLA bot in this particular case. A @finos/git-proxy-maintainers should be able to force merge this PR. Please email help@finos.org with any questions or concerns.

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.

There is a need to test a against the release publishing scripts before merging this as the @06kellyjac and I are both concerned that the changes to package.json will break import of types and running via the main entry.

I am unsure of the correct solution or why we modify the package when publishing...

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, my comments were dealt with. However, I'd appreciate @06kellyjac signing off on typescript config ;-)

@kriswest
Copy link
Contributor

To merge after we roll the next RC (which will hopefully be today)

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.

Still LGTM, will attempt to merge when checks are done

@jescalada
Copy link
Contributor

@finos-admin I believe this PR is ready to merge - only the EasyCLA check is missing!

CC: @kriswest

@kriswest
Copy link
Contributor

kriswest commented Oct 3, 2025

@06kellyjac could you approve this one then merge? Its still got you down as a requesting changes and is asking for a second reviewer:
image

Copy link
Contributor

@06kellyjac 06kellyjac left a comment

Choose a reason for hiding this comment

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

There's still some complications from what I can see

Also with 38 commits can we get a squash please, many thanks.

@kriswest
Copy link
Contributor

kriswest commented Oct 9, 2025

Plugin loader issues - we agreed to rip that out for now to be replaced. However, I'm not sure which PR that will/should happen in

@fabiovincenzi
Copy link
Contributor Author

@kriswest The plugin loader tests now pass locally, but the ./plugin export in package.json points to src/plugin.ts, which references source files. We could leave it as is for now. Alternatively, I can skip these three tests if you prefer.

@jescalada
Copy link
Contributor

@kriswest I'll do that in a separate PR - I'd suggest to just skip/remove the plugin test file since it's no longer relevant!

@kriswest
Copy link
Contributor

We had 3 action items from the last meeting #1211 related to this PR:

  • @fabiovincenzi to update build/publish scripts as per new TS dist folder conventions, removing legacy scripts and adjusting SED logic as discussed.
  • @06kellyjac to review build script/package.json changes once @fabiovincenzi has a PR up.
  • @jescalada to remove legacy plugin loader and tests, and document approach for v2.0 extensibility.

Presumably the alst action is a different PR, so this one can just skip the tests?

@kriswest
Copy link
Contributor

@fabiovincenzi was your slack comment indicating that this PR is ready for review again?

@fabiovincenzi
Copy link
Contributor Author

@fabiovincenzi was your slack comment indicating that this PR is ready for review again?

@kriswest Yes

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, but would like @06kellyjac to sign off as I trust his eyes on this better ;-)

fabiovincenzi and others added 3 commits October 17, 2025 13:54
Co-authored-by: Kris West <kristopher.west@natwest.com>
Signed-off-by: Fabio Vincenzi <93596376+fabiovincenzi@users.noreply.github.com>
Signed-off-by: Fabio Vincenzi <93596376+fabiovincenzi@users.noreply.github.com>
@06kellyjac 06kellyjac enabled auto-merge (squash) October 20, 2025 13:37
@06kellyjac
Copy link
Contributor

@finos-admin hi, I can't see a force merge option, only to auto-merge once the check is complete

prior conversation: #1142 (comment)

@06kellyjac 06kellyjac merged commit f216ae6 into finos:main Oct 22, 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.

[Setup] Add TypeScript to the Project

5 participants