nodejs.org icon indicating copy to clipboard operation
nodejs.org copied to clipboard

Refracto: update more code to ts

Open AugustinMauroy opened this issue 1 year ago • 1 comments

Description

Refracto to have more file in ts: it's allow to have type-check and it's "securise" our code

[!NOTE] There are still files written in javascript, either they are used in the next.config.js so they remain in js for the form but if SWC transpiles the ts. And there are also node scripts, but you'd need to have a loader or be in node 22. So for the moment I'd rather do nothing for those.

Validation

  • All test should pass
  • Website should display correctly (use Vercel preview)

Check List

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

AugustinMauroy avatar Oct 26 '24 15:10 AugustinMauroy

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

Name Status Preview Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview Oct 26, 2024 3:49pm

vercel[bot] avatar Oct 26 '24 15:10 vercel[bot]

FYI many of these files are intentionally done in plain JS -- so that they do not need to be transpiled by TypeScript.

There is no virtual benefit (necessarily) for having them in TypeScript. What's the motivation behind this change?

For me, it's ‘important’ to have them in typescript, for example next.dynamic, which is responsible for the entire content of the site. Because typescript offers type-checking which allows us to check that our code is correct and is supposed to help with the review. We shouldn't all write in typescript in ‘wow that's typescript’ mode but see it more as a tool to limit errors.

AugustinMauroy avatar Oct 29 '24 09:10 AugustinMauroy

FYI many of these files are intentionally done in plain JS -- so that they do not need to be transpiled by TypeScript. There is no virtual benefit (necessarily) for having them in TypeScript. What's the motivation behind this change?

For me, it's ‘important’ to have them in typescript, for example next.dynamic, which is responsible for the entire content of the site. Because typescript offers type-checking which allows us to check that our code is correct and is supposed to help with the review. We shouldn't all write in typescript in ‘wow that's typescript’ mode but see it more as a tool to limit errors.

Again, they should not be in TypeScript. There are reasons here, and I'm not going to approve a PR just because you feel that something needs to be refactored to TypeScript.

ovflowd avatar Nov 16 '24 13:11 ovflowd

before closing, can we get opinion from other members of the team @nodejs/nodejs-website

AugustinMauroy avatar Nov 23 '24 10:11 AugustinMauroy

I believe the main reasons I'm against TypeScript here are:

  • I'm the sort of maintainer who prefers TypeScript on userland (Apps), whereas plain JavaScript within Libraries or Configuration files.
  • Some of these files should be able to be used independently of TypeScript or anything behind Turbopack/Webpack; I don't see the need for TypeScript here. Otherwise, we need to introduce yet another build step.
    • It is the same reason why many files within the i18n package are plain JavaScript; we would introduce more dependencies.

This change is not a QoL update (IMO). Are these JSDocs fine, or is anyone having issues with them? I feel that having continuous refactoring is negative, and the time spent here on transforming these files into TypeScript could have been well spent elsewhere.

I don't necessarily have strong feelings here, but I won't remove my -1 until someone provides me a solid, convincing reason why TypeScript here would be better for these files, if not just a matter of preference.

ovflowd avatar Dec 13 '24 22:12 ovflowd

the time spent here on transforming these files into TypeScript could have been well spent elsewhere.

This is perhaps the best argument - we have a lot on our plate with finite time. Everything we do is an investment, and it's hard to see the dividends or losses right away. I will stop with the analogy.

Considering the lack of consensus, I think the proper course of action is to close. @AugustinMauroy are you okay with that?

bmuenzenmeyer avatar Dec 20 '24 04:12 bmuenzenmeyer

I am ok with that 👌

AugustinMauroy avatar Dec 20 '24 07:12 AugustinMauroy