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

fix: add support for type transformations to node runner #204

Merged
merged 12 commits into from
Mar 23, 2025

Conversation

nwidynski
Copy link
Contributor

@nwidynski nwidynski commented Mar 21, 2025

Unfortunately I noticed this feature was only introduced on the 6th minor release of v22 & v23. We should take this into consideration properly. https://nodejs.org/en/blog/release/v22.6.0 & https://nodejs.org/en/blog/release/v23.6.0

I also learned today, that Node has feature flags, which also include one for typescript. Sorry for not checking this earlier.

Copy link

changeset-bot bot commented Mar 21, 2025

🦋 Changeset detected

Latest commit: d9bdf47

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
synckit Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

codesandbox-ci bot commented Mar 21, 2025

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Copy link

pkg-pr-new bot commented Mar 21, 2025

Open in Stackblitz

npm i https://pkg.pr.new/un-ts/synckit@204

commit: d9bdf47

Copy link

codecov bot commented Mar 21, 2025

Codecov Report

Attention: Patch coverage is 50.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 97.41%. Comparing base (510edaa) to head (d9bdf47).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/index.ts 50.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #204      +/-   ##
==========================================
+ Coverage   96.26%   97.41%   +1.14%     
==========================================
  Files           1        1              
  Lines         241      232       -9     
  Branches      118      110       -8     
==========================================
- Hits          232      226       -6     
+ Misses          9        6       -3     

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

@nwidynski nwidynski changed the title fix: restrict type stripping to correct node minor versions fix: add support for type transformations to node runner Mar 21, 2025
@JounQin JounQin merged commit e13a68a into un-ts:main Mar 23, 2025
22 of 23 checks passed
@JounQin JounQin mentioned this pull request Mar 23, 2025
@nwidynski
Copy link
Contributor Author

nwidynski commented Mar 23, 2025

@JounQin I saw that you changed typeof NODE_TYPESCRIPT === 'undefined' to !NODE_TYPESCRIPT.

This is problematic because it will then also apply for supported versions if the flag isnt passed or no-experimental-strip-types is set. We only want to throw when the flag isn't present, aka in node versions prior to v22.10.

@JounQin
Copy link
Member

JounQin commented Mar 23, 2025

@nwidynski

image

Are you sure?

@nwidynski
Copy link
Contributor Author

nwidynski commented Mar 23, 2025

@JounQin Yes, when running v22.10 without any flags, this value will be false. This means we are currently throwing an error. In v23.6+ this flag will be false when --no-experimental-strip-types is added, but in that case we also don't want to throw and instead filter.

@JounQin
Copy link
Member

JounQin commented Mar 23, 2025

OK, I understand a bit now, NODE_TYPESCRIPT would be undefined on <22.10 but false on >=22.10 without any flags, right?

@JounQin
Copy link
Member

JounQin commented Mar 23, 2025

But shouldn't we add --experimental-strip-types for Node 20.6+?

I'm going to not using this feature flag.

@nwidynski
Copy link
Contributor Author

nwidynski commented Mar 23, 2025

@JounQin Yep, the drawback of using this flag is dropping support for >=22.6 & <22.10. I think its worth it for the convenience, but I can understand wanting to support those as well.

@JounQin
Copy link
Member

JounQin commented Mar 23, 2025

@nwidynski Please help to review https://github.com/un-ts/synckit/pull/211/files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants