module: allow type stripping in node_modules#63853
Conversation
|
Review requested:
|
marco-ippolito
left a comment
There was a problem hiding this comment.
I want to make sure the @nodejs/typescript is aligned with this change.
There have been multiple discussions about this and they have always blocked.
I'm afraid things have not changed from their side
c404bed to
ac69e2d
Compare
Remove the restriction that disallowed stripping types from files inside node_modules folders, so packages can ship TypeScript sources directly. Without this change, in a monorepo every workspace package has to ship JavaScript (or both JavaScript and TypeScript) to be consumable by another package, or its contents have to be manually copied to a non-node_modules directory before they can be loaded. Neither of these workarounds is ideal. The ERR_UNSUPPORTED_NODE_MODULES_TYPE_STRIPPING error is removed, and TypeScript files under node_modules are now stripped like any other TypeScript file. Signed-off-by: Yagiz Nizipli <yagiz@nizipli.com>
ac69e2d to
0ef075b
Compare
|
Indeed; unless this change is somehow restricted to local symlinked private:true packages, I don't think it's going to be viable to ever allow this - publishing raw TS should never be a thing (while TS syntax continues to evolve). |
is this the only way of landing this change? if yes, happy to change the PR. |
|
The current implementation already works with symlinked packages |
|
If that's the case, then wouldn't that mean it already works with monorepos? |
|
Yes, it does. We depend on it at work. |
|
I confirm it already works for monorepos that use symlinks. |
|
There’s a lot of discussion at #58429. I think the remaining “it doesn’t work with monorepos” feedback is specifically around |
|
@nodejs/typescript What if we put it behind a compile flag? If someone wants it so bad they can compile Node from source with their custom config |
|
I don't think anyone wants this enough that they're unwilling to use a more convenient solution, but are willing to compile Node.js themselves. |
|
Then that sounds like an issue with pnpm, specifically, and not an issue with node's implementation - perhaps that's where efforts should be focused? |
|
Also, it's already behind a runtime flag, so I very much doubt someone would prefer re-compiling Node.js rather than use the runtime flag |
While gate keeping an important feature, we can not just say "an issue with pnpm". We're the gatekeepers and if we are gatekeeping we have the responsibility to consider and handle every edge case. |
its not, currently there is no way to disable this behavior |
What about #58429 (comment)? |
that's a crazy workaround, I wouldnt say it's a "runtime flag" it's using a loader encoded in base64: |
|
Calling registering a loader a "crazy workaround" is a tad too far, wouldn't you agree? Call me crazy, but I refuse to believe someone would prefer compiling a different version of Node.js rather than use that workaround |
It's still a workaround, since it's sole purpose is to bypass the current behavior. The crazy part is to inline it in base64. I doubt someone is running it in production. But I imagine there are several companies running their custom built node that might want this. |
I don’t know if this workflow is specific to pnpm—it’s neither a package manager bug nor a Node.js bug, but I think it’s an unfortunate bad interaction in a pretty compelling use case. When you have a monorepo with a bunch of libraries consumed by an app, you need a way to package that app up for deployment. The way its dependencies are laid out in the monorepo doesn’t make sense for moving it to a server or packaging it into a CLI or baking it into a container image—they might be hoisted a few directories up, interleaved with devDependencies and dependencies needed only by other apps, and full of symlinks that might even resolve to a global store somewhere in your home directory. When I’ve worked on codebases like this at Microsoft, we would handle this by publishing all the libraries to a private internal package feed, copying the app into a Docker context by itself, and I think it would be great if we could make this scenario work, but the line we won’t move on is preventing package authors from publishing TypeScript-only packages to the npm registry, for reasons that have been reiterated many times in earlier discussions. What if Node.js allowed loading TS files from packages with |
|
Having a runtime flag would help with this. I don’t we can ship being on by default |
The main objection raised so far is preventing the publication of raw TypeScript to the npm registry. Packages marked as private are explicitly not intended for publishing, so why were they restricted in the first place? |
|
Previous attempt to enable it for private packages: |
|
I'd go for a runtime flag (for both public and private) disabled by default. And we also explicitly document that it is discouraged and likely never turned on by default. |
|
I think it'd be better for the runtime flag to only ever work for private. This isn't a "gatekeeping" thing - that implies the harm being held back is artificial - this is literally that it would be a catastrophe for the ecosystem if publishing untranspiled TS became common, and no feature or DX whatsoever is worth that outcome. |
I'm neutral on this. Allowing public packages while this flag stays off by default is something we can always revisit later. |
|
More context, because this comes up repeatedly:
I don't see any way a runtime flag gets added and it not eventually become defaulted on, leading to the trouble we've been trying to avoid. |
|
The point is that anything that enables publishing untranspiled TS must never be revisited. It's a nonstarter, and bringing it up ever again is always a waste of time. (until such time as the TS team decides to never again make a breaking syntax change, perhaps) |
If we make the flag work for private only packages, even if it was defaulted on (and I dont see this happening) it wont cause a catastrophe. |
|
@marco-ippolito i mostly agree - but i can see a world where people just set node_modules packages to |
Post install scripts are being disabled by default in npm 12 and other package managers, it requires enough configuration and machinery that is easier to just use a loader (and its still behind a flag so user must enable it) |
|
I agree on that, I don't think postinstalls would be viable. With that said, how do you plan to detect whether something is private? Node.js doesn't currently know what a package is, and the closest heuristic (find the closest package.json) can be abused by adding a private package.json file in a subfolder. |
The only way is with the closest package.json. I dont think your scenario is entirely viable as the entry point for the package still needs to be a js since it finds the package.json in the root (that needs to be private otherwise). I'll try to write a test to cover it. |
|
node definitely knows what a package is - only a package's package.json's |
|
That's only because you're going through a bare import, and Node.js is able to treat the resolved folder as the "package" root. If you do a relative import I think the heuristic is just the classic directory backtracking, and it's trivial to cloak the "true" package.json behind one in a subfolder. All you need is an entry point doing a relative import. Now, it is a little contrived, but it's simple enough that I could imagine it being shared around. |
You want to consume your local deps from source (i.e. with type stripping) ? Use symlinks. You want to consume your local deps from a copy inside |
|
@arcanis if it's a relative import (not counting reaching into node_modules, since that string should never appear in a specifier) it's not reaching into a package, so type-strippability should be the same as the file you're pulling from. |
This removes the restriction that prevented type stripping from working on files inside
node_modulesfolders, allowing packages to ship TypeScript sources directly instead of being forced to ship JavaScript.Motivation
When working on a monorepo, without this particular change, every package needs to ship JavaScript (or both JavaScript and TypeScript) for its files to be includable from another package, or the contents of the package need to be manually copied into a non-
node_modulesdirectory before they can be loaded. Neither of these workarounds is ideal: they add a build step (or a copy step) for code that Node.js is otherwise perfectly capable of running directly, and they defeat the purpose of built-in type stripping for internal workspace dependencies.With this change, a workspace package can simply point its
exportsfield at its.tssources and be consumed by sibling packages with zero build steps.Changes
lib: remove theisUnderNodeModules()check fromstripTypeScriptModuleTypes(), and remove the now-unthrowableERR_UNSUPPORTED_NODE_MODULES_TYPE_STRIPPINGerror.doc: moveERR_UNSUPPORTED_NODE_MODULES_TYPE_STRIPPINGto the legacy error codes section; rewrite the "Type stripping in dependencies" section to document the new behavior, recommend wiring.tsfiles through"exports", and point to the module compile cache for amortizing the stripping cost.test: flip the existingnode_modulesfailure expectations to success (.ts,.mts,.cts,importandrequire()paths, includingrequire(esm)), and add a new fixture covering a"type": "module"package that exposes.ts/.mtsfiles via"exports"subpaths.Notes