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

feat: support native type stripping for Node >= v23 #442

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jean-michelet
Copy link
Contributor

#441

My editor has reformatted the code a bit, but eslint seems ok with some of the changes.
I think the PR is still readable and it improves readability a bit, let me know if I should revert.


t.test('independent of module support', function (t) {
t.plan(8)
t.plan(typeStrippingEnabled ? 7 : 8)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can be removed when #440 is merged.

@mcollina
Copy link
Member

Can you add something in the README?

@Eomm
Copy link
Member

Eomm commented Jan 26, 2025

My editor has reformatted the code a bit, but eslint seems ok with some of the changes.
I think the PR is still readable and it improves readability a bit, let me know if I should revert.

If you run npm run lint:fix, does the change reverse itself?

@jean-michelet
Copy link
Contributor Author

If you run npm run lint:fix, does the change reverse itself?

Not the indentation, but remove semi-colons, use single-quotes etc.

@Eomm
Copy link
Member

Eomm commented Jan 26, 2025

If you run npm run lint:fix, does the change reverse itself?

Not the indentation, but remove semi-colons, use single-quotes etc.

mmm so how does npm run lint passes? cc @voxpelli

@jean-michelet
Copy link
Contributor Author

Probably because it doesn't enforce certain formatting rules, like trailing coma for example: https://github.com/fastify/fastify-autoload/pull/442/files#diff-93d89c5bddae92725a3ce7b3a38a424edb3d4f4de3b403e34af97dadee0d583eR11

Copy link
Member

@Fdawgs Fdawgs left a comment

Choose a reason for hiding this comment

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

Throwing in a block until I can properly review.

Copy link
Member

@climba03003 climba03003 left a comment

Choose a reason for hiding this comment

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

Too much stylistics changes.

Copy link
Member

@Fdawgs Fdawgs left a comment

Choose a reason for hiding this comment

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

As has been said, the stylistic changes need to be reverted as it's hard to see what functionality has actually changed.

@@ -319,6 +319,7 @@ provider, such as `ts-node`. Otherwise, widening the acceptance extension here w
```
## Override TypeScript detection using an environment variable
> **Note**: Node >= 23 supports native type stripping, and this plugin will automatically take advantage of it. Read more about native type stripping on the [API docs](https://nodejs.org/docs/latest-v23.x/api/typescript.html#modules-typescript).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
> **Note**: Node >= 23 supports native type stripping, and this plugin will automatically take advantage of it. Read more about native type stripping on the [API docs](https://nodejs.org/docs/latest-v23.x/api/typescript.html#modules-typescript).
This plugin uses [native type stripping](https://nodejs.org/docs/latest-v23.x/api/typescript.html#modules-typescript) with Node 23 and later.

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.

5 participants