nodejs / loaders

ECMAScript Modules Loaders
MIT License
126 stars 18 forks source link

Support typescript with --experimental-strip-types #208

Closed marco-ippolito closed 1 month ago

marco-ippolito commented 2 months ago

Add this issue to keep track of the work for supporting typescript out of the box, in the PR you can find documentation etc... The main idea is to use type-stripping. This is achieved ideally through the use of an external dependency, in my opinion @swc/wasm-typescript. I would like to have something basic and usable to propose to the public.

Tracking points to discuss:

Roadmap: https://github.com/nodejs/loaders/issues/217

legendecas commented 2 months ago

I don't think there are real downsides by going with full TS transform instead of just type stripping.

@nicolo-ribaudo has pointed out that the downside is that the painfully migration process on the transformation of runtime behaviors will be bound with Node.js LTS release cycles and likely longer, such as class fields.

jakebailey commented 2 months ago

useDefineForClassFields defaults to true when the target is set to ES2022 or higher; I believe all supported versions of Node.js exceed that version so I unless I'm mistaken, I think it's unlikely that particular item will affect anything put into Node.js in the future (except in the case that users want their tsconfig.json files to be respected as they do in tsx/ts-node/etc, which is a separate problem). The only reason this particular setting exisited is because TS added classes before they were standardized (same as decorators), so was needed to help maintain behavior.

marco-ippolito commented 2 months ago

I guess that given enough stability guarantees there is no issue supporting the whole typescript set, I'm not against it and I would be very happy not to make users adopt a subset. That said, I could suggest setting up something like a canary in the gold mine for typescript, where new versions are run against node?

magic-akari commented 2 months ago

When I read "It is possible to execute TypeScript code using --experimental-strip-types", I expect Node.js to be able to actually transform 100% of the TypeScript syntax, not a subset of it.

With the --experimental-strip-types flag, code can execute with the same semantics as TypeScript, which is my expectation for it. However, it cannot be the other way around.

The Type Annotations proposal is not TypeScript. I would have no problem if the feature is documented as "Type Annotations proposal support" and the transforms being applied to .js files. However, if it's documented as "TypeScript support" and applied to .ts files, then it should work 100% like TypeScript.

There are many JavaScript Runtimes in this world that only implement a subset of JavaScript features, but their source files still use the .js suffix. It is not reasonable to deprive them of the .js suffix just because they are missing some features.

For the same reasons, to me, the subset of TypeScript chosen by --experimental-strip-types can still be called TypeScript, and the use of the .ts suffix is reasonable.

The current doubts and questions are more related to the documentation. We need to clarify in the document what --experimental-strip-types actually means.

I really like the idea of --experimental-strip-types. It allows us to avoid transformations other than Type in a more direct way, thereby enhancing DX through a clean path.


Additionally, I would like to reference the motivation for this PR that has already been mentioned here, to express my views.

I don’t think anyone is advocating for type stripping to be the only way that Node supports TypeScript. There’s already the module customization hooks, that people can use with libraries like tsx or ts-node, and we have https://nodejs.org/en/learn/getting-started/nodejs-with-typescript to help people set those up. We absolutely want to make that easier and more automated, as discussed in https://github.com/nodejs/node/issues/43818 and https://github.com/nodejs/node/pull/49704, though we’re in the middle of redesigning the hooks per #198.

DanielRosenwasser commented 2 months ago

That said, I could suggest setting up something like a canary in the gold mine for typescript, where new versions are run against node?

We test TypeScript (at least) weekly across the top 800 TypeScript codebases on GitHub and across some well-known test suites, so more coverage is always appreciated - but I don't exactly know what you have in mind when you say running against Node. You mean running against .ts files in the Node.js repo?

jakebailey commented 2 months ago

That said, I could suggest setting up something like a canary in the gold mine for typescript, where new versions are run against node?

I'll also note that any grammar change in TypeScript will cause any bundled code to break for that syntax. For example, microsoft/TypeScript#58729 may introduce a new parameter modifier. Testing using the parser from the WIP PR to add this type stripping to Node.js gives:

/home/jabaile/work/strip-types/node_modules/@swc/wasm-typescript/wasm.js:253
            throw takeObject(r1);
            ^

  x Expected ',', got 'callback'
   ,-[1:1]
 1 |
 2 | declare function setTimeout(deferred callback: (args: void) => void, ms?: number): NodeJS.Timeout;
   :                                      ^^^^^^^^
 3 |
 4 | setTimeout(() => {});
   `----

So this previous commentary:

So really we’re faced with two options:

  1. Bundle a version of TypeScript that will quickly grow outdated, and let users override it via the customization hooks.
  2. Bundle type stripping which should never get outdated [...]

Are really the same without the override; vendoring will always lead to the same outdated-ness, and patching back will always be required to keep things up to date no matter what project is vendored to transpile. (This is why I was suggesting not bundling on the Twitter thread.)

This isn't really a semver concern, but rather a question of user expectation (which I'm sure we'll dig into at the call).

DanielRosenwasser commented 2 months ago

There are many JavaScript Runtimes in this world that only implement a subset of JavaScript features, but their source files still use the .js suffix. It is not reasonable to deprive them of the .js suffix just because they are missing some features.

You are right, but I think the context is important about when this happens. Off the top of my head:

For the same reasons, the subset of TypeScript selected by --experimental-strip-types can still be referred to as TypeScript, and it is reasonable to use the .ts suffix.

I completely get what you're saying, but I think there are shades of gray here and it depends heavily on the context. I think in some situations, it is fine to say "this tool only understands a subset of TypeScript". The TypeScript compiler even has modes like verbatimModuleSyntax, isolatedModules, and isolatedDeclarations to support these tools better.

But I think it would be very awkward for Node.js to draw a line at just type annotations in .ts files, and as others on this thread have noted, it would lead to a lot of confusion.

marco-ippolito commented 2 months ago

If we decide to bundle a tool, users will always be limited by the last version supported by the transpiling tool. And it's fine, if typescript ships a new feature and that our tool does not support, that can easily prevented by documenting the latest supported version until the tool supports it.

The problem is that if the new syntax/feature breaks existing code. Code written for Node v20 needs to work for the whole 3 years that the release is supported. We would have to lock a version for the whole time if no backward compatibility is guaranteed.

DanielRosenwasser commented 2 months ago

The problem is that if the new syntax/feature breaks existing code.

We really try to be careful about this. When we add a new feature that has the potential to break existing code, we typically apply similar heuristics to avoid conflicts with JavaScript code. For example, when adding the as keyword for type assertions, or the satisfies keyword for satisfies expressions, we conditionally parse if no line terminator exists between the prior expression and the keyword. So code like

let x = foo as (Bar)

parses as a single VariableStatement, but the following valid JavaScript and TypeScript code

let x = foo
as (Bar)

continues to parse as a VariableStatement followed by a call within a StatementExpression.

marco-ippolito commented 2 months ago

Thanks @DanielRosenwasser I really appreciate your follow up and I feel we are getting closer to a proper solution. What I meant with canary in the gold mine is a tool to make sure we keep backwords compatibility with existing typescript code supported by node. (Thinking out loud) This is what I have in mind as an example of what a collaboration might look like:

This would ensure that typescript can go forward with new features improving stability, while Node can keep adding support for new typescript feature without breaking existing user. This would also create a sort of "spec" for tools to be compliant with. Maybe its too large in terms of scope, but having a way to measure breakages since typescript does not follow semver would be massive. Idk if it makes sense, late night ideas 😴

GeoffreyBooth commented 2 months ago

I don’t think there are real downsides by going with full TS transform instead of just type stripping.

If the transforms are stable, and we can count on there being no breaking changes to them (or new transforms introduced) over a 3-year cycle, then Node supporting the transforms is a possibility. That was one of my questions above.

There are some downsides, though. Maybe they’re outweighed by the benefits, of course; everything is always a trade-off. Here are some downsides I see in supporting TypeScript transforms:

So basically, if we only support type stripping then we get a faster, more future-proofed TypeScript. We could decorate the error message thrown when encountering syntax that requires transformation, along the lines of “The enum syntax requires generating JavaScript code, which --strip-types cannot do; but you can add support for this syntax by installing the dependency tsx and running your app with --import=tsx“ and we can automate those suggested steps for the user. We could also add a second flag specifically to enable transforms, to opt into the performance hit, but the downside of that would be that we would never be able to remove SWC even after V8 supports Type Annotations.

This is also why I asked above about how the TypeScript team feels about transforms in general. Daniel wrote “We haven’t introduced new non-ECMAScript runtime constructs in years and steer away from them as much as possible.” If transforms are a de facto deprecated feature of TypeScript, then perhaps it’s not a bad thing to steer users away from them, where the easiest way to run TypeScript in Node is in the type stripping mode that provides the best performance, and the customization hooks mode can provide support for running these legacy syntaxes.

privatenumber commented 2 months ago

This feature is super exciting. But looking at the PR, it seems like there's quite a few caveats that it may be misleading to call it "TypeScript support".

To get CLI type-checking via tsc, you need to create a tsconfig.json file with allowImportingTsExtensions enabled, which requires noEmit to be enabled. Additionally, a few other configurations are implicitly required due to the nature of the Node runtime (e.g. verbatimModuleSyntax, allowJs, resolveJsonModules, etc).

Given this is motivated by TypeScript support being the most requested feature in the Node.js survey, I think it's crucial to emphasize in the release that only type support was added, and that this setup is not compatible with most existing TS codebases that rely on basic TS resolution (.js.ts), tsconfig.json for configuration, or tsc for compilation.

This is interesting because a leaner "runtime" flavor of TypeScript might emerge as a result. I'm not opposed to this, but it could lead to fragmentation within the Node.js TypeScript user base.

For this new workflow, it would be nice if TypeScript could release a lighter type-check only command that assumes these defaults so typed Node.js/Deno projects wouldn't need to create the same tsconfig.json file every time.

marco-ippolito commented 2 months ago

For this new workflow, it would be nice if TypeScript could release a lighter type-check only command that assumes these defaults so typed Node.js/Deno projects wouldn't need to create the same tsconfig.json file every time.

This is already possible from user space with external loaders, user could add a loader that performs type-checking during development with whatever tsconfig rules. Node.js currently does not support config files

This is interesting because a leaner "runtime" flavor of TypeScript might emerge as a result. I'm not opposed to this, but it could lead to fragmentation within the Node.js TypeScript user base.

This might happen, but I personally think that .ts is the correct file extension, at runtime the file has .ts extension, while the transformation .ts => .js should be performed by bundlers. The cost of runtime guessing the file extensions (running multiple stats on the fs to see if .js is a real .js or a .ts) is not good in production, its a big performance overhead, while its irrelevant during development. Node.js should only strip types and execute javascript rather then support all of ts caveats and configuration that can be done by user space tools.

privatenumber commented 2 months ago

Yeah, I completely agree with you (and I didn't mean to imply that Node should support implicit import resolution).

I mainly wanted to note that the feature documentation and release should be worded carefully to set expectations. The rest was just excitement towards how I imagine the landscape could evolve.

This is already possible from user space with external loaders, user could add a loader that performs type-checking during development with whatever tsconfig rules.

Sounds like you'd have to run the code to type check. Ideally, TS releases a lighter command to type check statically. This would be useful on CI when you want to lint the code without running it.

GeoffreyBooth commented 2 months ago

@DanielRosenwasser Thanks to you and your team for joining today’s call. My takeaways from the call are:

Does this match your perception? Am I missing anything?

If there are no specific notes around --experimental-strip-types, can we feel free to proceed with that PR or would you like more time to review it further?

arcanis commented 2 months ago

I think we’re willing to take the chance that users will find the increased performance worth the tradeoff of unsupported features

I still find the supporting research lacking. Given that the run-TS-transparently ecosystem already settled a long time ago on "run the TS transforms", I would expect the default implementation to be matching behaviour, not something new and confusing ("I'm following a TS tutorial but enums don't work, why is that?").

I also have more abstract concerns in the way this feature is being developed; it feels like we're kinda hijacking the development of a third-party project by making decisions about what "TypeScript support" means in their place (enums, but generally speaking by, pick-and-choosing language features). Yes, the flag you envision is --experimental-strip-types, but the loader would be tied to .ts extensions - it doesn't feel right to me that Node would ship an arbitrary subset and pretend it's TS.

The TypeScript team is worried about package authors publishing untranspiled TypeScript files to the npm registry. Others on the call consider this a ship that has already sailed.

It didn't sail since --experimental-strip-types hasn't sailed. If you talk about TS code already being published (I'm not aware of any amongst popular tools?), then I'd be curious to see a short list - if only to see if they use enums or not, and thus whether they'd be compatible with your approach.

mcollina commented 2 months ago

Node.js should only strip types and execute javascript rather then support all of ts caveats and configuration that can be done by user space tools.

I agree. This means that whatever subset of ts we support natively has to be compilable by tsc. This means that we should support importing “.js” files where a “.ts” file is present on disk.

rauschma commented 2 months ago

This is subjective but:

jakebailey commented 2 months ago

There are a few topics that I don't think we had time to address on the call (due to talking about other things; if we meet again it would be good to time box or have a list of specific topics ahead of time).


It seems like what users "want" (per comments before in this thread) is to be able to use the latest version of TypeScript. If the TS->JS code lives within Node.js, is there a method by which someone can upgrade that transform out of band? Otherwise, it seems like the only option is to go back to using loaders, or to eject and go back to transpiling on-disk.


I'll also note that I don't personally find arguments relating to the "type annotations" proposal to be convincing.

If the Type Annotations proposal graduates, then it will be implemented in V8 and a future version of Node will get type stripping for free. Then we can remove SWC from our pipeline [...]

This may not actually turn out to be true, depending on the way the proposal goes. At one point, the proposal worked like a "token soup" that allowed a wider range of freedom for future language changes, but the current spec encodes specific syntax that would be allowed. This means that if TypeScript ever adds anything new that doesn't fit that spec (e.g. satisfies, deferred parameter modifiers in 5.6), one can't simply treat all TS files as "JS with annotations".

Supporting TS with a type-stripper doen't seem like a gateway to "JS with annotations" in that way. By nature, vendoring SWC is just capturing a point-in-time TypeScript syntax, and would too continue to evolve.


As for TS extensions, theoretically that should all "work" via allowImportingTsExtensions, but it's definitely going to be a hazard to expect users to publish. You definitely do not want to have TypeScript load someone else's TypeScript code unless you're absolutely certain they are also using the same compiler options you are. We've already talked about the emit differences that can happen (class fields, decorators; cases where TS adopted something before it was standardizes), but what's also problematic from a user experience perspective is when modules resolve to someone else's TS code, leading to:

JSR and Deno were brought up in this context, but they are "special" in that for non-Deno users, JSR serves up .js and .d.ts files, avoiding the above, while Deno is strict about which options can even be set by nature of having a completely different config scheme.

The TypeScript team is worried about package authors publishing untranspiled TypeScript files to the npm registry. Others on the call consider this a ship that has already sailed, and doing things on our side to try to discourage the practice (such as trying to prevent TypeScript support in node_modules) are likely to have unwanted side effects such as breaking monorepo workflows.

I don't personally think the "ship has already sailed" on this front, solely from the perspecive that module resolution of existing packages could not have mapped to .ts files and actually been able to run. esbuild in particular takes care to not load .ts files in node_modules due to the previously mentioned gotchas, though by "only stripping", the emit differences do go away (leaving only the other two gotchas).

GeoffreyBooth commented 2 months ago

It seems like what users “want” (per comments before in this thread) is to be able to use the latest version of TypeScript.

This is already possible today: --import=tsx.

If the TS->JS code lives within Node.js, is there a method by which someone can upgrade that transform out of band?

The type stripping we’re proposing won’t do any transforming, just replacing types with whitespace (which maybe is what you’re considering a transform). No this would not be upgradable. We’ll update SWC with future versions of Node like any other dependency, and new TypeScript syntaxes that need to get added to SWC for stripping can be added in future versions of SWC and then into Node as a semver-minor update. Older versions of Node and SWC won’t be able to strip those new syntaxes and would error on them, just like old versions of Node and V8 error on new JavaScript syntaxes.

Otherwise, it seems like the only option is to go back to using loaders

The module customization hooks aren’t going away and are intended for the users who want “full” TypeScript, including being able to run the latest TypeScript version and configure it via tsconfig.json. Type stripping is an alternative, not a replacement.

You definitely do not want to have TypeScript load someone else’s TypeScript code unless you’re absolutely certain they are also using the same compiler options you are.

Yes, but this is already a hazard today and isn’t really affected by the type stripping proposal. I understand the TypeScript team’s desire to avoid the problems associated with publishing TypeScript to the npm registry, but I don’t think the type stripping proposal really makes things any worse than they already are; or what we could put in Node that would improve the situation without breaking valid use cases such as monorepos or users consuming their own local packages written in TypeScript.

Basically I’m not seeing any concerns with type stripping per se, just a general “we think users prefer what tsc or tsx have to offer.” Which is fine; but those tools already exist and will continue to exist, and the users who prefer them can continue to use them, so I’m not sure how relevant this feedback is to the type stripping proposal. If users don’t value the type stripping feature then they won’t use it, and everyone moves on, and that’s fine.

JakobJingleheimer commented 2 months ago

This means that we should support importing “.js” files where a “.ts” file is present on disk.

I vehemently disagree and I will block this to the utmost of my ability.

JakobJingleheimer commented 2 months ago

I like what Geoffrey is suggesting: if you want basic "node can read ts code", use this feature. If you want to get fancy, that takes a tinsy bit of effort.

Regarding old code bases being incompatible with the stripping feature because they containing erroneous file extensions within import specifiers, that seems a very real concern. I was just having this discussion in my own loaders lib, and I'm thinking to create a codemod (as a separate lib) to correct invalid .js.ts (and other flavours like .jsx.tsx). This would work for both this node feature and projects using tsc for type-checking (by leveraging the aforementioned tsconfig option). It is fully compatible with built-in features of VS Code as well.

jakebailey commented 2 months ago

The type stripping we’re proposing won’t do any transforming, just replacing types with whitespace (which maybe is what you’re considering a transform).

Yes, in my mind everything is just transforms. 😄

(This discussion was the first I had head the term "type stripping"; in tsc, the same pass also handles all other TS special syntax.)

Basically I’m not seeing any concerns with type stripping per se, just a general “we think users prefer what tsc or tsx have to offer.” Which is fine; but those tools already exist and will continue to exist, and the users who prefer them can continue to use them, so I’m not sure how relevant this feedback is to the type stripping proposal.

I think the reason it's getting brought up from different angles is just due to this being motivated by "TypeScript support" being on top of the survey. Type stripping only, .ts extensions, vendor only, the potential for loading .ts from node_modules, are all certainly the most expedient way to implement things, but I'm still generally worried that this may not match the user expectation of what "TypeScript support" "means".

GeoffreyBooth commented 2 months ago

I still find the supporting research lacking.

@arcanis I wanted to see how hard it would be to migrate an existing app to use --experimental-strip-types, so I migrated Corepack: https://github.com/nodejs/corepack/compare/main…GeoffreyBooth:corepack:strip-types

I got it as far as being able to print the help text when run with strip-types. To see for yourself, follow these steps:

# Check out the `--experimental-strip-types` branch and build it
git clone -b feat/typescript-support git@github.com:marco-ippolito/node.git
cd node
./configure
make
cd ..

# Check out the migrated Corepack
git clone -b strip-types git@github.com:GeoffreyBooth/corepack.git
cd corepack
npm install # Node wants a real node_modules folder; I didn't figure out Yarn PnP for this test
../node/out/Release/node --experimental-strip-types ./sources/_cli.ts # Prints the help text

Here’s the summary of what I changed:

The public class constructor update involved changing this:

export class Engine {
  constructor(public config: Config = defaultConfig as Config) {

to this:

export class Engine {
  config: Config;

  constructor(config: Config = defaultConfig as Config) {
    this.config = config;

The enum update involved changing this:

export enum SupportedPackageManagers {
  Npm = `npm`,
  Pnpm = `pnpm`,
  Yarn = `yarn`,
}

export const SupportedPackageManagerSet = new Set<SupportedPackageManagers>(
  Object.values(SupportedPackageManagers),
);

export const SupportedPackageManagerSetWithoutNpm = new Set<SupportedPackageManagers>(
  Object.values(SupportedPackageManagers),
);

// npm is distributed with Node as a builtin; we don't want Corepack to override it unless the npm team is on board
SupportedPackageManagerSetWithoutNpm.delete(SupportedPackageManagers.Npm);

to this:

export const SupportedPackageManagersEnum = {
  Npm: `npm`,
  Pnpm: `pnpm`,
  Yarn: `yarn`,
} as const;

export type SupportedPackageManagers = typeof SupportedPackageManagersEnum[keyof typeof SupportedPackageManagersEnum]
export type SupportedPackageManagersWithoutNpm = Exclude<SupportedPackageManagers, `npm`>;

export const SupportedPackageManagerSet = new Set<SupportedPackageManagers>(
  Object.values(SupportedPackageManagersEnum),
);

// npm is distributed with Node as a builtin; we don't want Corepack to override it unless the npm team is on board
export const SupportedPackageManagerSetWithoutNpm = new Set<SupportedPackageManagersWithoutNpm>(
  Object.values(SupportedPackageManagersEnum).filter((name) => name !== SupportedPackageManagersEnum.Npm),
);

This enum update went a little farther than was minimally necessary, because it bothered me that SupportedPackageManagerSetWithoutNpm was the same type as SupportedPackageManagerSet. You might want to consider making this change on its own merits, separate from any strip-types migration.

Anyway the bottom line is that this wasn’t all that hard, and a bit monotonous; yes @JakobJingleheimer we really need a codemod to add file extensions to import specifiers, and with { type: 'json' } to JSON imports. We also need to write a guide page to teach users how to do this themselves, including techniques for migrating particular syntaxes like enums.

I mentioned in https://github.com/nodejs/node/pull/53725#issuecomment-2227006441 that we should hold off on releasing --experimental-strip-types until we can improve the error messages and the docs/guides, and I feel much more strongly about that now. Some of the errors were borderline acceptable—TypeScript parameter property is not supported in strip-only mode—but RuntimeError: unreachable at wasm://wasm/0072633a:wasm-function[1259]:0x1a2e79 for all the missing file extensions is just ridiculous. Maybe we don’t need to fix all of these in the first PR, but we need these to be improved dramatically before we release this.

I also did a quick benchmark using the same node binary to compare --experimental-strip-types against tsx:

hyperfine --warmup 10 --runs 30 \
  '~/Sites/node/node --experimental-strip-types ./sources/_cli.ts' \
  '~/Sites/node/node --import=tsx ./sources/_cli.ts'
Benchmark 1: ~/Sites/node/node --experimental-strip-types ./sources/_cli.ts
  Time (mean ± σ):     152.8 ms ±   3.6 ms    [User: 321.8 ms, System: 29.2 ms]
  Range (min … max):   145.3 ms … 161.2 ms    30 runs

Benchmark 2: ~/Sites/node/node --import=tsx ./sources/_cli.ts
  Time (mean ± σ):     186.4 ms ±   2.6 ms    [User: 208.6 ms, System: 35.0 ms]
  Range (min … max):   179.3 ms … 191.7 ms    30 runs

Summary
  ~/Sites/node/node --experimental-strip-types ./sources/_cli.ts ran
    1.22 ± 0.03 times faster than ~/Sites/node/node --import=tsx ./sources/_cli.ts
yyx990803 commented 2 months ago

@GeoffreyBooth I think you are considering the migration "not that hard" because you are evaluating this from the perspective of an expert who has deep knowledge of Node.js, TypeScript AND this PR's particular implementation. I want to reiterate this is definitely not easy for an average user who just want to "run some TypeScript with Node.js", and catering to such more general users seems to be the original goal of this effort.

A common trap in tooling / framework design is that implementors often underestimate how much these "little inconsistencies" combined together can result in terrible UX.


Another topic I want to bring up is that it seems a major motivation of going with strip-type-only is the assumed performance advantage (and the ability to avoid source maps).

However, I think it's possible to implement a transform that:


As an example, enum can be transformed as:

In

enum Foo {
  X = bar(),
  Y = X,
}

enum Foo {
  Z = X,
}

Out

var  Foo = ((Foo, X, Y) => (
  X = bar(),
  Y = X,
Foo[Foo.X = X] = "X", Foo[Foo.Y = Y] = "Y", Foo))({});

     Foo = ((Foo, {X, Y} = Foo, Z) => (
  Z = X,
Foo[Foo.Z = Z] = "Z", Foo))(Foo);

Class constructor access modifiers:

In:

class Foo {
  constructor(public: foo) {
    // ....
  }
}

Out:

class Foo {
  constructor(        foo) { this.foo = foo;
    // ....
  }
}

The rare cases where columns could be affected are cases where the user has code in the same line with the closing } of enums:

enum Foo {
  X = bar()
} doSomething()

...or the opening { of the constructor body:

class Foo {
  constructor(public: foo) { doSomething() }
}

Which I think should be rare in practice - so in practice there should be very few mapping entries that need to exist for source maps, greatly reducing the overhead. By narrowing the use case to this specific case only, the transform could also be written in a way that doesn't even need to construct an AST (just directly do source manipulation as it parses). The performance could be as fast, if not faster than the current swc strip-type-only implementation.

Would you say the existence of something like this will make the TSC more willing to consider full TypeScript syntax support?

GeoffreyBooth commented 2 months ago

catering to such more general users seems to be the original goal of this effort.

I was the one who defined the original goal of this effort 😄 At least when we first starting discussing type-stripping a few weeks ago. The goal was to find something that we could ship within Node that would survive a three-year LTS period, despite TypeScript itself not following semver. At the time we assumed that TypeScript transforms would need to be ruled out because we assumed that they weren’t stable enough for a three-year lifespan, and fortunately the TypeScript team has informed us that that isn’t the case, so it’s an option to support them; but still, supporting them incurs a performance penalty. Perhaps less of a performance penalty if they can be transformed with the goal of trying to preserve line and column numbers, saving us from needing to generate a source map, but that’s a challenge for SWC and out of scope for Node.

Would you say the existence of something like this will make the TSC more willing to consider full TypeScript syntax support?

I can only speak for myself, not the TSC. I think you need to look at what some of the TypeScript team have been saying, though: “full” TypeScript support isn’t just type-stripping plus transforms, it’s also type checking and supporting all the options within tsconfig.json. Node can’t ever include all of that in core, because the type checking and tsconfig options very much do change within a 3-year cycle. So even if we support type stripping plus transforms, we’re still going to leave many users unhappy because of the other things that get left out.

And the solution for those users is to just use TypeScript in full, via tsc or --import=tsx or Vite or something similar. Those solutions already exist and will continue to exist, and Node will continue to support and encourage them; our error messages and docs will point people in their direction to get whatever features they want that Node can’t provide. Fundamentally it’s just not possible to satisfy both of the user requests of “full” TypeScript and no breaking changes for a three year LTS cycle. Either the TypeScript will get very out of date or it won’t be stable, and I consider both of those options to be worse than the two-option menu of type-stripping plus hooks for external tools.

I can imagine you asking “so if bundling the full TypeScript experience is off the table, what about the semi-full TypeScript of just type-stripping plus transforms?” And my reply is to just look at the work I did to migrate Corepack. Maybe 5% of it was dealing with the public class property and the enum, and I could’ve migrated the enum in a much simpler way than I did. Much more time was spent on updating import specifiers, though I’m sure that could be automated (and migrating most if not all syntaxes like enums could probably be automated too). In other words, transforms aren’t much of an issue, and they have modern equivalents that are strippable, so I think it’s a better tradeoff to ban them and get faster performance than support something that could be deprecated. It’s a certainty that any built-in TypeScript support that Node has won’t be able to run all existing TypeScript code, so if people are going to need to migrate at all (whether by hand or via a tool) they might as well migrate to the most performant syntaxes rather than Node sacrificing some performance to save some of the migration work. And finally, our target audience for this feature is users starting new projects, not users migrating existing ones, as the latter users by definition already have a solution that works for them. New users starting from scratch can work within the constraints of type stripping if that’s how they want to run their TypeScript, and presumably an ecosystem will grow with linting tools and the like for helping guide them. And for everyone else, as I keep saying, tsc and tsx and Vite and tsimp and ts-node and so on will continue to exist and continue to work, and continue providing fuller experiences than Node ever can.

yyx990803 commented 2 months ago

First of all, by "full syntax support" I mean a transform-only support, like what esbuild / swc / oxc does with isolatedModules: true. Never intended to want type-checking, tsconfig options etc.

Perhaps less of a performance penalty if they can be transformed with the goal of trying to preserve line and column numbers, saving us from needing to generate a source map, but that’s a challenge for SWC and out of scope for Node.

It's not out of scope. If such transform exists with negligible perf overhead compared to strip-types-only, then the argument for Node.js to go with strip-types-only becomes much weaker. In fact, I am raising this question because I am very confident this is something feasible, therefore the assumption that "strip-types-only has big perf wins" needs to be challenged.

Out of curiosity I ran the same benchmark on your modified corepack source using oxc-node:

Benchmark 1: node --import @oxc-node/core/register ./corepack/sources/_cli.ts
  Time (mean ± σ):      93.4 ms ±   1.4 ms    [User: 99.8 ms, System: 13.4 ms]
  Range (min … max):    91.4 ms …  96.7 ms    30 runs

Benchmark 2: node --import tsx ./corepack/sources/_cli.ts
  Time (mean ± σ):     136.0 ms ±   0.8 ms    [User: 161.1 ms, System: 21.4 ms]
  Range (min … max):   134.0 ms … 137.5 ms    30 runs

Summary
  node --import @oxc-node/core/register ./corepack/sources/_cli.ts ran
    1.46 ± 0.02 times faster than node --import tsx ./corepack/sources/_cli.ts

The perf is 1.46x over tsx vs the 1.22x of current --experimental-strip-types. Granted this is native binary vs wasm, but this one is actually running a full transform with source maps, and even respecting resolution config in tsconfig.json. Even both as native binary, an oxc-based position-preserving + no AST allocation TS transform can very possibly out perform other strip-type-only implementations. SWC can also implement the proposed transform on top of the current strip-type-only implementation and compare the result.

In other words, transforms aren’t much of an issue

I fundamentally disagree with this. Removing these small discrepancies avoids a Node-flavored subset of TS. Right now these discrepancies are justified in the name of performance - but as I said, the assumption that "strip-types-only has big perf wins" needs to be challenged.

and presumably an ecosystem will grow with linting tools and the like for helping guide them.

This is encouraging yet another flavor of TS that only works in Node, creating more fragmentation / confusion in an already complicated landscape, but the other side of the tradeoff is questionable for the reasons outlined above.

"Node.js supports full syntax-level TS transforms, but not type checking / tsconfig" is a much cleaner definition than "Node.js supports a subset of TS syntax". It aligns with all the other transforms users are already using via esbuild / swc / oxc etc.

My whole argument is that it is not a good idea to intentionally steer away from possible ecosystem alignment in the name of performance, especially when the performance gain might not be as significant as imagined.

jakebailey commented 2 months ago

I think you need to look at what some of the TypeScript team have been saying, though: “full” TypeScript support isn’t just type-stripping plus transforms, it’s also type checking and supporting all the options within tsconfig.json

I don't think anyone has actually advocated for checking; that is of course the bit that "doesn't follow semver", though the general scheme is that "any change could be a breaking change". The config defaults are what change in semver major, and the API is otherwise very stable. Not that using TS itself here is a goal; any third party emitter can handle TS code if it complies with isolatedModules.

I mean a transform-only support, like what esbuild / swc / oxc does with isolatedModules: true. Never intended to want type-checking, tsconfig options etc.

Note that there are a couple of options respected by those tools which affect emit, namely useDefineForClassFields (and target after a specific version) and experimentalDectorators. These aren't affected by isolatedModules, and I'm not sure what swc does at this point (I think it leaves define class field semantics on by default, differing to TSC?)

GeoffreyBooth commented 2 months ago

My whole argument is that it is not a good idea to intentionally steer away from possible ecosystem alignment in the name of performance, especially when the performance gain might not be as significant as imagined.

Sure. Right now, though, the transpiler you describe that can do transforms yet still not need source maps doesn’t exist, as far as I know. If someone builds that, and we measure the performance difference and it’s truly negligible, then it’s something to consider. We can always add support for transforms later, even after this goes stable.

But my point about looking at the changeset for the Corepack migration was that transforms account for very little of it, and the rest will still remain even if we support transforms: adding file extensions, adding with { type: 'json' }, using import type, etc. There are also some potential migrations that Corepack didn’t need but that many apps would, of avoiding tsconfig paths and adjusting to ESM-syntax code running as ESM rather than being transpiled into CommonJS (so no global require intermixed with import/export, etc.). All of these Node-specific idiosyncracies would remain even if we start supporting transforms. Maybe some of them we might find ways to get rid of, but I doubt we’d eliminate all of them.

And so then yes, Node’s built-in support will support a particular subset of TypeScript’s full feature set, and users wanting to use that support instead of tsc or tsx or Vite etc will need to live within those limitations. Probably everything that would need to change could be automated, from adding file extensions to splitting import/import type to migrating from tsconfig paths to package.json "imports" and so on. And if that’s the case, I’m not sure why transforms are such a concern as compared with any of the other things that ecosystem tools might support that Node might not.

yyx990803 commented 2 months ago

If someone builds that, and we measure the performance difference and it’s truly negligible, then it’s something to consider. We can always add support for transforms later, even after this goes stable.

Great. That's pretty much what I want to hear.


Surely there will be some migration costs that cannot be covered by transforms, but don't we agree that the smaller the differences are, the better?

More importantly we need to evaluate these differences in context - things like tsconfig paths are much easier to explain because Node.js' own module resolutions rules need to be the source of truth, but explaining that

class Foo {
  construct(public id: number) {}
}

...works as expected everywhere else but just not in Node.js (which claims to "support TypeScript") seems... very awkward.

marco-ippolito commented 2 months ago

@kdy1 do you think is possible to perform transformation of ts features and preserve the position in the file with whitespacing?

arcanis commented 2 months ago

@arcanis I wanted to see how hard it would be to migrate an existing app to use --experimental-strip-types, so I migrated Corepack: https://github.com/nodejs/corepack/compare/main…GeoffreyBooth:corepack:strip-types

I didn't ask whether the migration was hard to someone well versed in Node.js. I did ask however:

As for whether the migration would be easy or hard, that matters much less to me than having a migration. As designed, the feature would force people out of specific TS syntax constructs, which seems an overreach to me.

kdy1 commented 2 months ago

@marco-ippolito It's partially possible, but it's a bit hard in general.

Basic restrictions are

We need to evaluate each syntax under this assumption.

Enums

Enums are typically written as


enum Foo {
    a = 1,
    b = 2, c = 3
}

and it can be transpiled as

(function(Foo) {
    Foo[Foo["a"] = 1] = "a";
    Foo[Foo["b"] = 2] = "b"; Foo[Foo["c"] = 3] = "c"; 
})(Foo || (Foo = {}));

so it can be supported.

Decorators

The decorator pass is too complex to be supported with this kind of transpilation. There's no way to preserve token order.

Constructor properties

I'm not sure if this is possible or not, but my guess is that it's impossible in some cases, meaning trying to support it may result in forcing specific formatting for the users.

If you have some ideas, please share

yyx990803 commented 2 months ago

@kdy1 I shared an example here: https://github.com/nodejs/loaders/issues/208#issuecomment-2227176737

Since decorators are on standards track and already stage 3, I don’t think they need to be covered by the TS transform in the long run. Short term it may be the only exception.

marco-ippolito commented 2 months ago

I'm willing to support full ts features, behind a flag, if we see it works well and it's stable, we will as well remove the flag and set it to default. Example:

node --expertimental-strip-types --enable-ts-transform  foo.ts

And with source maps support

node --expertimental-strip-types --enable-ts-transform  --enable-source-maps foo.ts

The current swc version does not support this so it's gonna be in a next iteration. If we can make it work without source-maps and with whitespacing it would be even better

GeoffreyBooth commented 2 months ago

Who is this feature built for? What is the intended audience?

Why is this feature designed by default as a departure from status quo?

Why have other tools been able to support transforms, and Node.js would be unable to?

Why is Node.js taking ownership of the syntax allowed in a .ts file?

What are examples of packages shipped to run as TS, and do they use enums?

I don’t know of any. As the TypeScript team has explained, shipping packages written in TypeScript is generally a bad idea, unless you’re in control of your consumers (for example, if it’s meant for use only by you in your own app, or for the members of your team). Even if there are packages out there that don’t use enums, they probably don’t follow all of the rules of --experimental-type-stripping (see the things I changed in Corepack). The goal isn’t to support existing published packages written in TypeScript, if there are any.

jakebailey commented 2 months ago

The proposed behavior matches the Type Annotations proposal, which might become a future status quo (and which many of us support). If that occurs, then type stripping will just become standard JavaScript, and code written for type stripping today will work in any runtime including browsers; and Node will be able to simplify our internals by removing SWC, since V8 would handle the type stripping for us.

Type stripping is primarily inspired by the Type Annotations proposal, of which TypeScript’s @DanielRosenwasser is a champion. This is arguably just as blessed a syntax or standard as the moving target of whatever a given version of tsc supports.

I would still shy away from this as a motivator; the Type Annotations proposal may not cover all TS syntax. Not only that, but it can't, because in TypeScript the expression foo<number>(value) is a function call with type arguments, but in JavaScript that's two comparisons. This means that the type annotations proposal will never contain all valid TypeScript syntax today. As such --experimental-strip-types would a different subset of the language (so not "matching" the TC39 proposal), and can't be eventually replaced by something from V8.

marco-ippolito commented 1 month ago

Since this has landed and we have a roadmap I'll close this issue, we can talk about next steps here https://github.com/nodejs/loaders/issues/217

thesoftwarephilosopher commented 1 month ago

@marco-ippolito I want to help as much as I can to get this feature working thoroughly before 2025. I have nothing but free time. Please let me know how I can help.