build: enable typescript sources #60489
Conversation
|
Review requested:
|
nodejs-github-bot
added
lib / src
legendecas
left a comment
legendecas
left a comment
There was a problem hiding this comment.
How does this handle type-stripping at build-time? It seems to me the PR only handles it with NODE_BUILTIN_MODULES_PATH.
|
I screwed up the commit lol 💀 Im going to push the rest when I get home. |
| getLazy, | ||
| isUnderNodeModules, | ||
| kEmptyObject | ||
| } = require('internal/util'); |
There was a problem hiding this comment.
Be aware that in a TS file, these kinds of imports will be entirely untyped because require is just a function that returns any and is not special-cased in TS files where you're supposed to use import util = require("internal/util").
There was a problem hiding this comment.
Could the types be detected via something like } = /** @type {import('internal/util')} */(require('internal/util'))? Perhaps using more explicit paths in the @type.
There was a problem hiding this comment.
Technically yes, though it's casting any to something so would be "unsafe" (but very easily identifiably correct).
Related-ish: microsoft/TypeScript#60032 (I can't find a better issue about const = require in TS, strangely)
There was a problem hiding this comment.
The main reason not to do this is because it only gives you the value side of the import, so you can't use them in type positions.
There was a problem hiding this comment.
Sorry could you explain this in a bit more detail? Both what's wrong with my suggestion and what you would recommend instead, given the constraint that we can't use typescript syntax yet.
There was a problem hiding this comment.
Here's a fleshed out example of what happens in a TS file: Playground Link
This comes down to the fact that require the function is "just a function" when in a TS file; the binder does not bind it in such a way that we can go look up types.
We do this in JS files, though it is somewhat limited.
We've talked about doing this in TS files, but IIRC there are problems with doing so.
There was a problem hiding this comment.
Okay, but I was suggesting the JSDoc syntax in a .js file; you say that tsc will “do this“ in JS files, so would my suggestion work?
And if not, that’s fine, but is there something else you could recommend? Even if we migrate Node internals to TypeScript, that’s probably a very long timeline effort and so we’ll have a lot of JS files for a long time. Getting type checking working on JS files in the meantime would be a win.
There was a problem hiding this comment.
This PR is converting some of the JS to TS, which is why I was bringing it up in this TS file. If you leave things in JS, then you don't need to think about this, of course.
There was a problem hiding this comment.
I think we would like to add type checking to the Node codebase. Obviously that can be done by converting to TypeScript syntax, or adding JSDoc types to JavaScript; or a mix of both. We'll likely have a latter or a mix for a long time. Given that constraint, is there a way to add type checking to the files we have now?
There was a problem hiding this comment.
Yes, I'm sure you could use JSDoc (as clunky as it is).
|
Along the lines of #60489 (comment), perhaps a preliminary step would be to get type checking working for the files that already exist? And we should gradually add JSDoc types for the parts of the codebase that currently lack them. Personally I find type checking to be the valuable addition, and whether the sources are written in TypeScript syntax or JavaScript-with-JSDoc types syntax is a secondary concern. We could get type checking working first, with what we have, and figure out supporting |
|
I started some PRs a while ago to start that, but got sidetracked (and forgot 😅). So a fair amount of it may already be working for the esm submodule. |
|
FWIW I think if we want to do this, ideally the type stripping process should not have a Node.js dependency, or else there will be a chicken and egg problem, especially on cross-compiled builds or builds on new architecture/platforms (I think that will take a substantial amount of gyp voodooing to make it possible for our CI). |
|
It'd be nice if the type stripping weren't within Wasm but instead an actual native dep, which IIRC is likely to be come possible if/when V8 gains the Rust dep for temporal anyway... |
Thats true, I just realized you need node to strip types to ... build node 😆 |
|
I noticed I cannot reopen the PR because I force pushed |
V8 14.1 in Node.js 25 has already switched to the Rust dep for Temporal (
|
8 participants
I know this might be super controversial, but let's give it a try.
With this PR we enable typescript to be used in core modules + typechecking.
The .ts files are stripped at build time so no runtime overhead.
The issue is that you cannot build without amaro.
My approach was quite hacky I'm sure there are better ways to handle this, but I wanted to see what people think.
The pros are static types in node core and the possibility of shipping our own types instead of relying on @types/node.
It is possible to gradually migrate like showed in this PR a module from js to ts without having to rewrite everything
@nodejs/tsc @nodejs/typescript