Review requested:

• @nodejs/build
• @nodejs/gyp
• @nodejs/security-wg
• @nodejs/tsc

@marco-ippolito do you know what steps are needed to run and expose Rust code just like C++?

@marco-ippolito do you know what steps are needed to run and expose Rust code just like C++?

its already done in this PR, see d9ee95f

basically add the crate in the deps/crates cargo.toml, create a header file if doesnt come with the crate and add it to the node gyp

Will this type strip at runtime? Or will it do it at build time?

Will this type strip at runtime? Or will it do it at build time?

Build time

How does this interact with the --node-builtin-modules-path flag? will it still type strip at runtime in that case?

How does this interact with the --node-builtin-modules-path flag? will it still type strip at runtime in that case?

Yes, I tested it, it works fine. it plugs in the js2c module

I'm relatively concerned about adding 1 million and 300 hundred thousand lines of code, even if they are just dependencies.

Is there a solution that does not require this much?

I dont think so since we are vendoring dependencies. I think the only solution is to have an automation generate them but its kinda impossible to review anyways. (The temporal PR from @legendecas had the same problem)

I think the question should be is it necessary to have every swc dependencies to be included?

I listed the newly added dependencies, and the followings are the biggest deps that I doubt if they are required:

9.6M	deps/crates/vendor/tracing          => Diagnostic purpose
5.1M	deps/crates/vendor/libc             => Indirect dep of crates `cpufeatures` and `num_cpus`
2.8M	deps/crates/vendor/regex-automata   => Regex engine
1.6M	deps/crates/vendor/bitvec           => Indirect dep of swc_sourcemap
1.6M	deps/crates/vendor/zerocopy         => Indirect dep of hashbrown
1.3M	deps/crates/vendor/zerocopy-derive  => Indirect dep of hashbrown
1.0M	deps/crates/vendor/idna             => Indirect dep of crate `url`

Additionally, crates like wasm-bindgen are not technically needed for the PR purpose.

I think technically many of these deps can be stripped down.

Can I just like delete them and see if it builds?

I tried to remove those crates but it seems they are all required to build. Idk if there is a way to know exactly which ones are unused or how to remove them, I'm no rust expert

This is blocked by nodejs/build#4245

Node.js configure: Found Python 3.12.3...
INFO: configure completed successfully
SKIP_XZ=1 supplied, skipping .tar.xz creation
mkdir -p out/doc
cp doc/node-config-schema.json out/doc
mkdir -p out/doc/api
cp -r doc/api out/doc

added 323 packages, and audited 325 packages in 6s

136 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities
[09:05:04.899] ERROR: Unknown file extension ".wasm" for /home/runner/work/node/node/tools/doc/node_modules/@minify-html/wasm/index_bg.wasm
TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".wasm" for /home/runner/work/node/node/tools/doc/node_modules/@minify-html/wasm/index_bg.wasm
    at Object.getFileProtocolModuleFormat [as file:] (node:internal/modules/esm/get_format:189:9)
    at defaultGetFormat (node:internal/modules/esm/get_format:232:36)
    at defaultLoad (node:internal/modules/esm/load:145:22)
make: *** [Makefile:845: out/doc/api/addons.html] Error 1
    at async ModuleLoader.loadAndTranslate (node:internal/modules/esm/loader:543:45)

this seems unrelated but consistent 🫤

I'm relatively concerned about adding 1 million and 300 hundred thousand lines of code, even if they are just dependencies.

Is there a solution that does not require this much?

I had she same concern in another PR I was working on. (no spoiler ;P)
I wonder if it would make sense to track such deps in another repo or similar solutions.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.65%. Comparing base (3725bd2) to head (5c971a2).
⚠️ Report is 2 commits behind head on main.

@@            Coverage Diff             @@
##             main   #62146      +/-   ##
==========================================
- Coverage   89.65%   89.65%   -0.01%     
==========================================
  Files         676      675       -1     
  Lines      206543   206492      -51     
  Branches    39547    39532      -15     
==========================================
- Hits       185184   185133      -51     
+ Misses      13480    13474       -6     
- Partials     7879     7885       +6     

I really don't know how it's possible to review a PR with 3000+ files changed manually.

I don't think that this can be anything more than a proof of concept, unless there is some reliable way to review it automatically.

This is not a proof of concept. We are vendoring dependencies, so unless we are blocking all future rust dependencies (and as other comments suggest, also other contributors are already working on adding more), we have to find a way around. A way to review is by commit. A separate commit adds the vendored dependencies, it is true that its impossible to review that single commit.

I'm assuming that there hasn't been a formal discussion beyond what was previously discussed at #60489?

Aside from the deps question, I do think this warrants a bit more of a think from the design perspective.

What are we hoping to accomplish here? Huge swathes of core aren't really TS-able due to legacy patterns that we can't move away from (the old pseudoconstructors etc), so we would be committing to a divided codebase. Typechecking in TS modules would only be robust if all their JS module dependencies were typed as well, which is currently very limited.

tsserver-compatible IDEs support strict typechecking in JS, as would using tsc in a CI pipeline if we really wanted to. It seems to me that aiming towards a JSDoc-driven internal type system (with phased-in checkJs support) would be far less hassle than shoehorning in TS transpilation, is basically an extension of what some subsystems are already doing, and would be better compatible with the pre-ES6 elements in core.

The old pseudoconstructor pattern can be refactored to es6, I have done that myself a few times. We are using type stripping so it can be all migrated because it can coesist with javascript. The migration can take place over many years and its not a mass rewrite. Type checking applies only to the files we migrate over time.
The main benefit other than well types, is that at some point we can generate an accurate @types/node.
There are still things we need to figure out in type checking but those can be fixed as we go

The old pseudoconstructor pattern can be refactored to es6, I have done that myself a few times.

Moving to new-only construction of streams, EventEmitters, etc. is one of those "break all the legacy libraries" changes that I somewhat assumed we would never be able to make.

This is one example. The biggest caveat, however, is the module system. tsc has require(...) and module.exports as typechecked operations in JS only, and the typechecked equivalents in TS (import ... = require(...)/ESM-style imports and export =/ESM-style exports) are not erasable syntax. Jake did raise this in the previous PR, but we are going to be continually straining against a TS language service that simply wasn't designed with the internal module environment in mind, and would probably have to add type casts to every imported identifier just to hack around these constraints. (The file renamed in this PR is a case in point: it might "type-check" just fine, but the type of the lazy import is any, as would be the type of require('internal/util/colors') if imported from another core module.)

I'm all for better code quality in core, but this seems like a very nuclear move with more nuance than is maybe being presented.

The main benefit other than well types, is that at some point we can generate an accurate @types/node.

This comes up frequently as a suggestion. For reasons that are very complex and to do with the TS ecosystem rather than Node.js itself, autogenerating @types/node is never really going to work. It was the web team who last sat down with TS a year or so ago to discuss the ins and outs of this, I am happy to do so again, but this should not really be a consideration in weighing up the pros and cons of this change.

Moving to new-only construction of streams, EventEmitters, etc. is one of those "break all the legacy libraries" changes that I somewhat assumed we would never be able to make.

It needs to go through a deprecation cycle.

Also just because we cannot typecheck everything, it's not worth to adding it at all seems like a poor argument. The implementation needs to be refined on many levels but still it's worth exploring and not be killed on some speculations.

autogenerating @types/node is never really going to work

You are presenting this as an absolut truth. Can you argument it please

It was the web team who last sat down with TS a year or so ago to discuss the ins and outs of this, I am happy to do so again, but this should not really be a consideration in weighing up the pros and cons of this change.

Yes Im happy to discuss it again

Why are the new deps needed if amaro which is already vendored can already do ts -> js stripping?

Why are the new deps needed if amaro which is already vendored can already do ts -> js stripping?

Amaro needs javascript to run, it would require node as a build requirement to build itself. This is a pure rust/c++ implementation

Amaro needs javascript to run, it would require node as a build requirement to build itself.

Sure, but that seems like a viable option? That's what many projects do
That would avoid having two different ts implementations in the tree

Also, the underlying lib really needs just wasm not the Node.js js api so a separate runner could be used sharing most of the logic on v8 + wasm but prior to js being built - that won't even need a separate Node.js...

I just checked there is nothing requiring Node.js process actually in amaro
I.e. nothing that can't be faked with bootstrap js code
WebAssembly support is native in v8 and doesn't need additional actions

chalker@macbook-air _test % cat amaro.cjs
const amaro = require('amaro');
const { code } = amaro.transformSync("const foo: string = 'bar';", { mode: "strip-only" });
console.log(code);

chalker@macbook-air _test % node amaro.cjs 
const foo         = 'bar';
chalker@macbook-air _test % npx @exodus/test --engine=v8:bundle amaro.cjs
Engine: v8:bundle, running on ~/.jsvu/bin/v8
# amaro.cjs
const foo         = 'bar';
All 1 test suites passed
Total time: 185.111ms

This was executed on v8 cli. The code was bundled though, but that could be solved with some minor js bootstrap.
And v8 and amaro are already in-tree.

Perhaps replace Buffer.from with Uint8Array.fromBase64 where available 🙃
Apart from that only utf8 encoding/decoding is needed, and that's pretty much it

Also just because we cannot typecheck everything, it's not worth to adding it at all seems like a poor argument.

This is exactly my point, though. We already have a way of leveraging tsc/tsserver to provide robust typechecking in JS, in a way that can be gradually phased in (@ts-check directives), without any requirement for transpilation, without any new dependencies, and without necessitating breaking changes. I really don't see why we wouldn't want to go down this avenue instead (essentially #60489 (comment)).

@marco-ippolito this works on pure v8 cli:

Without bundling, with v8 CLI import()
(which could alternatively be replaced with just concatenating it into the script)

(async function() {
  class TextEncoder {} // only constructed, not actually used
  class TextDecoder {
    constructor(encoding = "utf-8", options = {}) {
      if (encoding !== 'utf-8' || !options.ignoreBOM) throw new Error('Unexpected')
    }
    decode(input = new Uint8Array(), options) {
      if (!(input instanceof Uint8Array) || options) throw new Error('Unexpected')
      return decodeURIComponent(escape(String.fromCharCode.apply(String, input)));
    }
  };

  const Amaro = globalThis.module = {}
  globalThis.require = (arg) => {
    if (arg === 'util') return { TextEncoder, TextDecoder }
    if (arg === 'node:buffer') return {
      Buffer: {
        from: (x, encoding) => {
          if (encoding === 'base64') return Uint8Array.fromBase64(x)
          throw new Error('Unexpected')
        }
      }
    }
    throw new Error('Unexpected')
  }

  await import('./node_modules/amaro/dist/index.js')

  const { code } = Amaro.exports.transformSync("const foo: string = 'bar';", { mode: "strip-only" });
  console.log(code);
})();

chalker@macbook-air _test % ~/.jsvu/bin/v8 tempout.cjs
const foo         = 'bar';

@marco-ippolito this works on pure v8 cli:

Without bundling, with v8 CLI import().

(async function() {
  class TextEncoder {} // only constructed, not actually used
  class TextDecoder {
    constructor(encoding = "utf-8", options = {}) {
      if (encoding !== 'utf-8' || options.fatal || !options.ignoreBOM) throw new Error('Unexpected')
    }
    decode(input = new Uint8Array(), options) {
      if (!(input instanceof Uint8Array) || options) throw new Error('Unexpected')
      return decodeURIComponent(escape(String.fromCharCode.apply(String, input)));
    }
  };

  const Amaro = globalThis.module = {}
  globalThis.require = (arg) => {
    if (arg === 'util') return { TextEncoder, TextDecoder }
    if (arg === 'node:buffer') return {
      Buffer: {
        from: (x, encoding) => {
          if (encoding === 'base64') return Uint8Array.fromBase64(x)
          throw new Error('Unexpected')
        }
      }
    }
    console.log('arg', arg)
  }

  await import('./node_modules/amaro/dist/index.js')

  var { code } = Amaro.exports.transformSync("const foo: string = 'bar';", { mode: "strip-only" });
  console.log(code);
})();

If we could run this in the js2c.cc, we could skip completely the rust dependency

Moving to new-only construction of streams, EventEmitters, etc. is one of those "break all the legacy libraries" changes that I somewhat assumed we would never be able to make.

The blast radius of this would be way too great to justify the effort on the ecosystem side. There is a lot of code that depends on this pattern, and updating it all is going to be problematic: even if we deprecate, most developers won't know how to deal with it because it's buried under 10 levels on dependencies on a module that hasn't been updated since 2016. Unless there is a clear reason (like security in the case of Buffer), I would not recommend we do this.