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

Update tsx example #6750

Merged
merged 1 commit into from
Jun 2, 2024
Merged

Update tsx example #6750

merged 1 commit into from
Jun 2, 2024

Conversation

GeoffreyBooth
Copy link
Member

@GeoffreyBooth GeoffreyBooth commented May 30, 2024

Description

This updates the tsx example to remove the mention of --loader, which is an undocumented experimental flag that was created by accident and will likely go away in the future, as the warning states when you use it. The stable, documented, recommended approach is to use --import instead. See nodejs/node#51196 (comment).

ts-node doesn’t (directly) support --import, so I’m leaving its example as just npx ts-node; there are multiple open issues on their repo asking them to update. I updated the tsx example to be more user-focused and to use tsx as the example of registering module hooks via --import.

Validation

https://nodejs-org-git-fork-geoffreybooth-remove-loader-openjs.vercel.app/en/learn/getting-started/nodejs-with-typescript#running-typescript-code-with-tsx

image

Related Issues

Resolves nodejs/node#51196

Check List

  • I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • I have run npm run format to ensure the code follows the style guide.
  • I have run npm run test to check if all tests are passing.
  • I have run npx turbo build to check if the website builds without errors.
  • I’ve covered new added functionality with unit tests if necessary.

@GeoffreyBooth GeoffreyBooth requested a review from a team as a code owner May 30, 2024 03:59
Copy link

vercel bot commented May 30, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview May 30, 2024 4:00am

@AugustinMauroy
Copy link
Contributor

Another point I'm not a fan of is that only one loader (tsx) is featured, whereas before there were several that worked very well. I think tsx could be the example, but we need to come up with other alternatives.

@GeoffreyBooth
Copy link
Member Author

Another point I’m not a fan of is that only one loader (tsx) is featured, whereas before there were several that worked very well. I think tsx could be the example, but we need to come up with other alternatives.

Previously it listed ts-node, tsx and “nodejs-loaders“. We still discuss the first two; the third didn’t get a description or link. Assuming it’s https://www.npmjs.com/package/nodejs-loaders, this is a package with 3 weekly downloads and it’s not specific to TypeScript.

Also this page isn’t about demonstrating how loaders work; the loaders docs do that. The purpose here is to explain to people how to use TypeScript.

@AugustinMauroy
Copy link
Contributor

I agree with you that nodejs-loaders is not a very good example.
But it's also important to avoid the learn putting too much emphasis on a package, because we need to keep in mind that we have a certain status.

@GeoffreyBooth
Copy link
Member Author

But it’s also important to avoid the learn putting too much emphasis on a package, because we need to keep in mind that we have a certain status.

I’m not sure what this means or what you’re asking me to change to resolve this note. The current page already emphasizes ts-node and tsx pretty strongly, and that remains in this PR. Maybe that should change, but the purpose of this PR is just to update the tsx example to remove the outdated reference to --loader. I agree that there could be other general improvements to this page, but perhaps those can come as follow-ups?

Copy link

github-actions bot commented Jun 2, 2024

Lighthouse Results

URL Performance Accessibility Best Practices SEO Report
/en 🟢 98 🟢 100 🟢 100 🟢 91 🔗
/en/about 🟢 98 🟢 100 🟢 100 🟢 91 🔗
/en/about/previous-releases 🟢 99 🟢 100 🟢 100 🟢 92 🔗
/en/download 🟢 100 🟢 100 🟢 100 🟢 91 🔗
/en/blog 🟢 100 🟢 100 🟢 96 🟢 92 🔗

Copy link

github-actions bot commented Jun 2, 2024

Unit Test Coverage Report

Lines Statements Branches Functions
Coverage: 92%
90.67% (593/654) 76.08% (175/230) 94.57% (122/129)

Unit Test Report

Tests Skipped Failures Errors Time
131 0 💤 0 ❌ 0 🔥 5.888s ⏱️

Copy link
Member

@mikeesto mikeesto left a comment

Choose a reason for hiding this comment

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

LGTM. I think it's important that we remove this reference to an undocumented & experimental flag that has already caused some confusion. We can discuss broader changes to the page elsewhere.

@AugustinMauroy
Copy link
Contributor

but perhaps those can come as follow-ups?

Yeah sure

Copy link
Contributor

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

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

LGTM ! Thanks for updating learn section

@ovflowd ovflowd added this pull request to the merge queue Jun 2, 2024
Merged via the queue into nodejs:main with commit b631f5b Jun 2, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants