Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type Stripping in node_modules/*/*.ts #14

Open
karlhorky opened this issue Oct 13, 2024 · 23 comments
Open

Type Stripping in node_modules/*/*.ts #14

karlhorky opened this issue Oct 13, 2024 · 23 comments

Comments

@karlhorky
Copy link

karlhorky commented Oct 13, 2024

Copied from original discussion in nodejs/loaders#217 (comment)


I know that as a user I won't have much sway here, but I think I'm not alone in thinking that files in node_modules/*/*.ts should also be allowed --experimental-strip-types type stripping.

A few thoughts:

1) Node.js Alone Won't Prevent TS npm Publishing

There is a lot of movement in the "execute TypeScript, don't build it" ecosystem already, for example with the advent of many ecosystem projects such as:

Publishing .ts files to npm and other registries is already a growing pattern, which drastically reduces the effort and complexity of publishing a TypeScript package.

I think as these projects evolve and more people get to know about this possibility, there will be a greatly increased interest in this possibility.

It's my opinion that this banning of node_modules/*/*.ts in Node.js alone won't be able to hold back the energy in the ecosystem, especially with working alternatives. (not to mention users circumventing this by using any of the other projects, which do not have this limitation)

2) User Experience

My first experience with node --experimental-strip-types was creating an internal checking tool which was its own package, without a build step.

I immediately fell flat on my face with this, because of the banning of node_modules/*/*.ts files.

Even though I was already somewhat aware of the discussions and objections behind this (eg. the nodejs/TSC meeting notes from 2024-07-24), for my use case it felt like an arbitrary limitation imposed on my project.

I imagine that as type stripping becomes more widespread, this will be a common complaint from users.

3) Node.js Features Available Everywhere

@GeoffreyBooth wrote about supporting a Node.js feature everywhere:

If we support a file type, we support it everywhere. Just as module detection needs to apply within node_modules so that a package that works locally continues to work when published to npm, the same applies to TypeScript. We shouldn't be limiting our functionality to try to encourage best practices.

This resonates with me - it seems like supporting a Node.js feature in as many places as possible will be the best for users and cause the least amount of support complaints.

Banning node_modules/*/*.ts to try to avoid a pattern which the ecosystem may adopt seems like a weak reason. (not to mention that the ecosystem may end up adopting this pattern anyway by finding cowpaths around this)

4) Identifying and Addressing Downsides

Downsides as mentioned by Ryan and Daniel on the TypeScript team and vocal node_modules/*/*.ts opponents such as Matteo are important and should be considered.

But it feels like also:

  1. Each problem should be carefully thought through, examined, clearly enumerated and documented (also asked for by Geoffrey)
  2. Once clearly enumerated and documented, solutions should be also thought through, with each solution's tradeoffs being also enumerated and documented

This being done with a dispassionate, neutral stance, not trying to prove any one viewpoint.

In reading through the publicly-available documents, I don't think this has been done yet.

5) At Least Allow Users to Choose

Even if careful enumeration of problems and solutions leads to the final conclusion that the tradeoff is not worth it for Node.js, I think it would be good to allow users to choose for themselves and disable this node_modules/*/*.ts banning with a flag, eg:

node --experimental-strip-types-in-dependencies

6) Similarities to .ts Rewriting Limitation

Oh one more thing, more related of a feeling than fact-based:

This node_modules/*/*.ts banning feels like it has a similar vibe to the .ts -> .js rewriting decision in TypeScript, which was overturned after years of doubling-down on .js imports (ironically heavily inspired by Node.js type stripping).

@marco-ippolito
Copy link
Member

marco-ippolito commented Oct 13, 2024

Adding a flag to allow type stripping in node_modules is trivial.
For a real solution the issue is how does node recognize if its published npm package or a monorepo type project?
If there was a marker in the package.json that would be possible.
Immagine an entry in the package.json "local_node_modules": true that prevented publishing to npm (when you try npm publish it would throw) but if node finds it, it allows type stripping even in the node_modules.

@JakobJingleheimer
Copy link
Contributor

If there was a marker in package.json that would be possible.

Isn't there? The namespace


I'm in favour of allowing publication of ts in general (as I'm also in favour of publishing jsx) because it allows the consumer to decide the output and avoids make-work steps and a game of telephone.

@aduh95
Copy link

aduh95 commented Oct 13, 2024

At Least Allow Users to Choose

I think that's possible, with loader hooks, Marco have been working on exposing the type stripping utilities for this exact use-case IIRC.

@karlhorky
Copy link
Author

karlhorky commented Oct 13, 2024

how does node recognize if its published npm package or a monorepo type project?

@marco-ippolito I think maybe first needs to be identified:

  1. What is the purpose of banning published npm packages with .ts? (What are all of the enumerated downsides and their symptoms?)
  2. Should these downsides / banning also apply to packages published to other registries? (eg. private registries or TS-native registries?) If yes/no, why?
  3. What are possible solutions or improvements to those downsides?

It seems like trying to ban users from publishing .ts files is maybe the wrong thing to be putting effort into?

Maybe better would be 4) Identifying and Addressing Downsides, listing out the real downsides which occur as a result of publishing .ts files, and then try to work on those problems / symptoms?

@JakobJingleheimer
Copy link
Contributor

It is possible with a loader hook: https:/jakobjingleheimer/nodejs-loaders#jsx--tsx

(This is about to be split into a monorepo and published separately as @nodejs-loaders/tsx)

@marco-ippolito
Copy link
Member

marco-ippolito commented Oct 13, 2024

Btw as @aduh95 mentioned, if parser.stripTypeScriptTypes nodejs/node#55282 lands, its possible to use an external loader that supports type_stripping in node_modules with the same output as node. amaro (should) already cover this use case when used as a loader

@mcollina
Copy link
Member

I agree that we should document the downsides of not supporting .ts files inside downloaded npm packages are not documented, and it would be great if @DanielRosenwasser and the rest of the typescript team voiced their concerns. They are the same as mine, just expressed in a much more precise form. I could take a stab at writing those concerns if they prefer (for them to review). This is important.

Summing them up are:

  1. TypeScript is designed so that only .d.ts files are shared across projects. They are interface files.
  2. Assuming a significant portion of the npm ecosystem will be replaces by ts-only modules, it would slow down massively our IDEs, because the numbers of parsed files would increase (10-100x?).
  3. Allowing only-TS dependencies will force a single tsconfig.json in the ecosystem.

@hverlin
Copy link

hverlin commented Oct 13, 2024

modern TS-native registries like JSR

jsr allows ts files, but has some restrictions (https://jsr.io/docs/about-slow-types#typescript-restrictions).
Following them might help to address the performance issues. (There is also the isolatedDeclarations option)

@jakebailey
Copy link

jakebailey commented Oct 13, 2024

My first experience with node --experimental-strip-types was creating an internal checking tool which was its own package, without a build step.

I immediately fell flat on my face with this, because of the banning of node_modules/*/*.ts files.

Theoretically, this shouldn't happen for plain monorepo use; the determination of "is in node_modules" considers the realpath of a file. But if you aren't using a monorepo tool with symlinks, I can see how this would fail.

Publishing .ts files to npm and other registries is already a growing pattern, which drastically reduces the effort and complexity of publishing a TypeScript package.

The reduction in complexity here is from the tooling, though; something like jsr handles the downleveling and d.ts emit for you behind the scenes. Just publishing TS code itself doesn't reduce much complexity in a world which really does need to have/run .js files... (I think it has the potential to actually increase complexity, without some careful consideration.)

jsr allows ts files, but has some restrictions (jsr.io/docs/about-slow-types#typescript-restrictions).
Following them might help to address the performance issues. (There is also the isolatedDeclarations option)

These kinds of restrictions do not help published TS source; TypeScript will load all of the source code given to it, including function/class bodies, expressions, etc. Publishing and loading .d.ts files strips away all of those interior bits and allows TypeScript to do less work.

"Slow types" is a bit of a misnomer becuase it's talking about "slow" in terms of generating declaration files from the perspective that the "slow types" require running tsc to typecheck. isolatedDeclarations encodes a list of rules that prevent needing type checking, and is therefore "fast", but the speed at which declaration files are emitted does not change the fact that loading d.ts files is faster than actual source code. It'd be nice if JSR's docs were modified to not call them "slow types" anymore.

There are other concerns here as well:

  • If users don't set --skipLibCheck (which, they arguably shouldn't in order to understand when their deps are broken), then they're going to be checking all of that extra source code too, including fully-annotated code.
  • Additionally, a package's tsconfig may not have the same settings as the user; depending on the differences, new errors may appear that wouldn't be there if .d.ts files are consumed instead (e.g., exactOptionalPropertyTypes, noUncheckedIndexAccess, etc, in the consuming package). This is also why transforming someone else's TS code is dangerous; there are options that some other package may have set which would affect emit, e.g. decorator use (including uses that are not exported!), useDefineForClassFields, etc.
  • Publishing TypeScript source may limit downstream users abiliy to not use the same TS version as you. Today, one runs tsc (or similar), leaving behind only JS (not read by tsc), and .d.ts without anything from expresion space. This means that TS is free to introduce a feature like satisfies and it'll be backward compatible for users of your package as the satisfies keyword does not appear in d.ts files. If you publish source, the version of TS you use to typecheck your code impacts downstream users.
    • This is technically no different than adding ES features, except that basically all TS syntax is erasable.
    • TS may also eventually remove something or be forced to change the emit of something altogether, e.g. to deal with enum, a new module keyword in JS, and so on.

All of these problems are handled by transforming your code before publishing it.

(My list is not exhaustive either; a proper combined list would of course be good.)

@marco-ippolito
Copy link
Member

What if we supported running in node_modules only if the package.json containts the "private": true property https://docs.npmjs.com/cli/v10/configuring-npm/package-json#private which prevents a package being published.
This way you still use monorepos but wont disrupt the ecosystem

@karlhorky
Copy link
Author

karlhorky commented Oct 14, 2024

Downsides of Publishing .ts to npm

So if I may try to make a simplified list of problems:

  1. Loading .ts source code is slower than loading d.ts files
  2. Checking .ts source code in node_modules without --skipLibCheck is slow
  3. Publishing newer / different TS syntax than the consumer tsconfig.json / TS version causes failures (similar to publishing newer ES syntax than consumer supports)

(happy to edit this or for someone to take over with a new list, just trying to make a simplified list)

Metrics

It would be cool to get some symptoms (metrics) on these things.

For example, with a small sized project that has 10 direct dependencies (and a number of transitive dependencies), and considering that 100% of the dependency graph converted to TypeScript, what would the concrete impacts be in latest VS Code on a 2020 mid-range computer?

By metrics I mean something like these fake numbers:

  1. Loading .ts source code is slower than loading d.ts files
    • VS Code startup time: +1s (+80%)
    • TypeScript extension startup time: +2s (+20%)
    • RAM: +200MB (+10%)
    • Auto-completion response time: +0.5s (+15%)
  2. Checking .ts source code without --skipLibCheck is slow
    • Full project build time: +2s (+15%)
    • Type checking for 10 dependencies: +1s (+10%)
    • RAM usage during type checking: +50MB (+5%)
    • Incremental builds time increase: +0.5s (+5%)

Update: @andrewbranch did some research and produced some similar metrics here:

.d.ts / .ts

Files:           268 / 317
LOC:             49k / 62k
Instantiations:  11k / 46k ❗
Memory:          92 kB / 118 kB
Parse time:      .76 s  / .91 s
Check time:      1.48 s / 3.02 s ❗
Total time:      2.58 s / 4.43 s ❗

@karlhorky
Copy link
Author

With clear metrics, it will be more clear what the tradeoffs are and what could be solved, optimized or worked around to minimize those downsides.

Some projects may decide that the tradeoff is worth it:

  • 👎 decreased performance, compat considerations
  • 👍 no build setup, configuration, maintenance, CI step

@electrovir
Copy link

What if we supported running in node_modules only if the package.json containts the "private": true property

I personally have, and I've seen many other projects have, mono-repos with npm workspaces (packages) that are not private, they are published as individual packages. The mono-repo's top-level package.json does set "private": true though.

@marco-ippolito
Copy link
Member

marco-ippolito commented Oct 14, 2024

What if we supported running in node_modules only if the package.json containts the "private": true property

I personally have, and I've seen many other projects have, mono-repos with npm workspaces (packages) that are not private, they are published as individual packages. The mono-repo's top-level package.json does set "private": true though.

Its a trade-off. If you want to run .ts in your node_modules, then you should do it locally.
private ensures that the package cannot be published.
This will work for some use case (projects that do not need publishing on NPM).

@targos
Copy link
Member

targos commented Oct 14, 2024

There's another problem with publishing .ts source code: It must be compatible with the tsconfig flags of the consumer (strict, verbatimModuleSyntax, etc.).

@karlhorky
Copy link
Author

karlhorky commented Oct 14, 2024

@targos yeah, that was mentioned above a couple of times, including my list above:

  1. Publishing newer / different TS syntax than the project tsconfig.json / TS version (similar to publishing newer ES syntax) causes failures

I'll reword to be more clear

@robpalme
Copy link

robpalme commented Oct 14, 2024

On the list of reasons @jakebailey stated for why redistributable TS on npm is a cause for concern, I will add my agreement (and expand) on a couple of specific points raised regarding future compatibility of TS syntax.

TS may also eventually remove something or be forced to change the emit of something altogether, e.g. to deal with enum, a new module keyword in JS, and so on.

  • enum might become a JS keyword if it comes back for more discussion at TC39. It was only blocked last time on a technicality - the inability to frame the problem statement - rather than any lack of a desire for the functionality. At the meeting last week I spoke to one of the folk who previously pitched it for Stage 1 and they remain interested. There are active discussions about permitting more syntax sugar in the JS language that might ease the ability to progress such features in future.

  • The legacy module keyword for TS namespaces is under consideration for future deprecation and several steps have already been taken towards dissuading further usage. There are a bunch of module-related JS language features in progress and at least one might use this keyword.

These are just possibilities. It's hard to say whether they will play out. Regardless, it's very cheap for Node to be conservative and avoid complicating future evolution. We just need to avoid third-party redistributable code (i.e. npm deps) depending on Node supporting that syntax. Happily --experimental-strip-types already side-steps both of these cases 👍

@jakebailey
Copy link

What if we supported running in node_modules only if the package.json containts the "private": true property docs.npmjs.com/cli/v10/configuring-npm/package-json#private which prevents a package being published.
This way you still use monorepos but wont disrupt the ecosystem

I don't understand why this helps; if the package is private, then it can't have come from a registry. If you're in a monorepo and doing this, then the package can only have been local and could be symlinked and not need anything to work today.

@jakebailey
Copy link

It would be cool to get some symptoms (metrics) on these things.

@andrewbranch did the analysis and shared his results here about a year ago: https://x.com/atcb/status/1705675335814271157

@karlhorky
Copy link
Author

@jakebailey thanks! Updated my post above with those numbers.

@benjamingr
Copy link
Member

With numbers, I'm -1 on transpiling/stripping in node_modules, I'm fine with having an opt-in flag in the package.json if we can reliably prevent published packages from transpiling but since we have a workaround (a loader + exposing parser.stripTypeScriptTypes) I'd personally prefer a better default (not transpiling node_modules) and having users opt in with the loader.

@robpalme
Copy link

To be clear, Andrew's numbers are comparing the cost of checking TS vs DTS. It seems fair to conclude we should continue encouraging distribution of DTS files in packages for faster checking of dependencies. That will help keep tsc and IDE usage fast.

This could be considered somewhat independent of whether the executable code in the package is TS or JS.

@anthonyshew
Copy link

(@karlhorky messaged me wondering if I had an opinion so just going to jot it down here.)

TL;DR: I pretty much agree with what TS core team is saying above. The way I'm building monorepos today, I wouldn't need this flag.

My high-level thought these days on internal packaging for monorepos is: Compile like you're shipping to npm*, even though you're not.

I find this simplifies the mental model. Your package consumers are going to look into node_modules to find the symlinks for the internal package's source code anyway**, so we can treat that boundary the same way, whether the package happens to be coming from npm or your monorepo. You then get to lean into the assumptions TypeScript makes about node_modules, the way Node.js approaches resolution, etc., instead of working against the ecosystem. While its convenient to export TypeScript from a package sometimes, I've been starting to change my tune about some of the tradeoffs.

As far as what's proposed in this issue goes, I admittedly don't have much of an opinion. There are a lot of different ways to build a monorepo today, many of which I'm not fond of. I try to teach folks my little happy path that I've carved out, but there are still plenty of valid, wonky things you can do, in the true spirit of JavaScript. If one were to follow the way that I happen to teach Workspace-building, this flag would be unnecessary, because there would be .js and .d.ts in each package, ready to be executed as if they came from npm.

*: There are exceptions to this idea, like dual-publishing. In your monorepo, you're in control, so you don't necessarily have to cater to the unknown matrix of consumers of a public npm package. Example: you know your monorepo is committed to using ESM, so you don't have to dual-publish.

**: I'm working from the assumption that you're not using compilerOptions#paths to share your packages amongst your workspace, properly using your package manager instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests