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

refactor: Add TypeScript support #929

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

fabiovincenzi
Copy link
Contributor

@fabiovincenzi fabiovincenzi commented Feb 28, 2025

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

Copy link

linux-foundation-easycla bot commented Feb 28, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link

netlify bot commented Feb 28, 2025

Deploy Preview for endearing-brigadeiros-63f9d0 ready!

Name Link
🔨 Latest commit 773754e
🔍 Latest deploy log https://app.netlify.com/sites/endearing-brigadeiros-63f9d0/deploys/67ed49bd04e4040008a48d6d
😎 Deploy Preview https://deploy-preview-929--endearing-brigadeiros-63f9d0.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Mar 9, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.04%. Comparing base (83e814b) to head (773754e).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #929      +/-   ##
==========================================
- Coverage   61.88%   59.04%   -2.84%     
==========================================
  Files          49       48       -1     
  Lines        1805     1680     -125     
==========================================
- Hits         1117      992     -125     
  Misses        688      688              

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

@JamieSlome
Copy link
Member

@06kellyjac - can we get a review and confirm whether this would constitute a patch or minor bump?

Any opine welcome @jescalada too 👍

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.

These suggestions would ensure npx @finos/git-proxy continues to work.

Although the package in npm wouldn't work properly if someone is doing require("@finos/git-proxy") (or import) from plain JS. We probably want to build out valid JS with type defs for the published package

@06kellyjac
Copy link
Contributor

Squashing the commits down would be appreciated 🙂

@JamieSlome
Copy link
Member

@fabiovincenzi - can you take a look at the comments and we can get this merged?

@fabiovincenzi fabiovincenzi force-pushed the typescript-setup branch 2 times, most recently from a3f23bb to e7ec595 Compare March 19, 2025 15:47
Copy link
Contributor

@jescalada jescalada Mar 20, 2025

Choose a reason for hiding this comment

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

@JamieSlome @06kellyjac I was wondering if we want to continue refactoring the project into ESM or just keep the CommonJS for now.

I've tried refactoring various modules to TS and ESM in subsequent PRs. However, integration with Mocha tests and some ESM-only libraries (such as the load-plugin library in /src/plugin.js) causes many headaches with failing tests. I think converting into TS but keeping imports in CommonJS would be the least time-consuming.

On top of that, despite ESM being the "latest and greatest" way of doing things, there's a lot of resistance in the JS ecosystem because of integration problems with other libraries. Node 22 fixes these compatibility issues, but I'm unsure if forcing git-proxy users to use Node 22 is acceptable.

TL;DR: Should we convert to ESM and bump Node to 22 (to fix compatibility issues in test runs), or keep CommonJS and only refactor modules into TS?

I'd appreciate your thoughts on this! 😃

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the input. I do enjoy a good upgrade but since 20LTS has security support for 1 more year and 18LTS still has 1 month it'd probably be sensible to stick to commonjs for the time being.

If we're building out javascript from our typescript for the published NPM package I'm thinking sticking with commonjs shouldn't be much of a problem for us.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you!

@fabiovincenzi
Copy link
Contributor Author

Hey @06kellyjac, @JamieSlome,

I've addressed all the comments and cleaned up the commit history by squashing the relevant commits.
Let me know if there's anything else needed to get this merged!

Thanks! 😊

@JamieSlome
Copy link
Member

@06kellyjac - are we ready to merge?

What is the recommend version bump here, minor or patch?

@06kellyjac
Copy link
Contributor

No, I don't think this is importable from javascript as-is. We'll need to build out js and type definitions for the published package.

@fabiovincenzi
Copy link
Contributor Author

No, I don't think this is importable from javascript as-is. We'll need to build out js and type definitions for the published package.

Hey @06kellyjac,

Could you clarify what you mean by "not importable from JavaScript as-is"? I thought git-proxy was being executed directly through the index.js, as the package.json doesn't have a "main" entry.

Also, do you want us to generate the JS and type definitions and include them in the publishing workflow for npm? Let me know how you'd like to proceed.

Thanks! 😊

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.

4 participants