Refracto: update more code to ts #7148
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
For me, it's ‘important’ to have them in typescript, for example |
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. |
There was a problem hiding this comment.
I appreciate your effort here, but sorry, I don't want this to get merged.
We shouldn't keep refactoring things on a whim, and there are reasons why certain things are not in TypeScript, they are internal utils and configuration files and hardly benefit anything from TypeScript.
|
before closing, can we get opinion from other members of the team @nodejs/nodejs-website |
There was a problem hiding this comment.
There are certainly parts of this PR that need polish (typos), TODOs, but I guess I'd like to better understand why more TypeScript is bad. I see some typings in here improving the code quality, while others simply shorten imports or replace jsdoc.
There is value here on both sides, but I don't yet understand Claudio's point of view.
| // NOTE: this file canot be a ts file | ||
| // because `nodevu` din't have typedefs |
There was a problem hiding this comment.
| // NOTE: this file canot be a ts file | |
| // because `nodevu` din't have typedefs | |
| // NOTE: this file cannot be a ts file | |
| // because `nodevu` didn't have typedefs |
| }); | ||
|
|
||
| const blogFeedEntries = posts | ||
| .filter(post => post.categories.includes(category)) | ||
| .map(post => ({ | ||
| id: post.slug, | ||
| title: post.title, | ||
| // @TODO: use righ object type for date | ||
| // wait https://github.com/nodejs/nodejs.org/pull/7143 to be merged |
There was a problem hiding this comment.
this is now out of date
| author: post.author, | ||
| date: new Date(post.date), | ||
| link: `${canonicalUrl}${post.slug}`, | ||
| })); | ||
|
|
||
| // @ts-expect-error - The addItem need a valide Item type |
There was a problem hiding this comment.
| // @ts-expect-error - The addItem need a valide Item type | |
| // @ts-expect-error - The addItem need a valid Item type |
| type RouteSegment = { | ||
| locale: string; | ||
| pathname: string; | ||
| }; | ||
|
|
||
| /** | ||
| * This is a list of all static routes or pages from the Website that we do not | ||
| * want to allow to be statically built on our Static Export Build. | ||
| * | ||
| * @type {Array<((route: import('./types').RouteSegment) => boolean)>} A list of Ignored Routes by Regular Expressions |
There was a problem hiding this comment.
this sorta implies their is a type we can import rather than define right?
| // Renders a Link Component for `a` tags | ||
| a: Link, | ||
| // Renders a Blockquote Component for `blockquote` tags | ||
| blockquote: Blockquote, | ||
| // Renders a CodeBox Component for `pre` tags | ||
| pre: MDXCodeBox, | ||
| // Renders an Image Component for `img` tags | ||
| // @ts-expect-error - MDXImage is a wrapper of Next Image so it's didn't have same props as HTMLImageElement |
There was a problem hiding this comment.
| // @ts-expect-error - MDXImage is a wrapper of Next Image so it's didn't have same props as HTMLImageElement | |
| // @ts-expect-error - MDXImage is a wrapper of Next Image so it doesn't have same props as HTMLImageElement |
|
I believe the main reasons I'm against TypeScript here are:
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. |
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.jsso 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
Check List
npm run formatto ensure the code follows the style guide.npm run testto check if all tests are passing.npx turbo buildto check if the website builds without errors.