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

module: allow running .ts in node_modules if private #55385

Closed

Conversation

marco-ippolito
Copy link
Member

@marco-ippolito marco-ippolito commented Oct 14, 2024

Refs: nodejs/typescript#14

This PR checks when attempting to run .ts files under node_modules if the package.json contains the property private: true.

This should make possible to use strip-types in monorepos, without allowing package maintainer to publish their packages on npm without compiling first.

Also not very performant since is checking/parsing the package.json every time a file under node_modules is encountered, it requires some sort of caching

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/typescript

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Oct 14, 2024
@marco-ippolito marco-ippolito added the strip-types Issues or PRs related to strip-types support label Oct 14, 2024
@mcollina
Copy link
Member

This solution works for me.

@@ -3041,7 +3041,8 @@ import 'package-name'; // supported
added: v22.6.0
-->

Type stripping is not supported for files descendent of a `node_modules` directory.
Type stripping is not supported for files descendent of a `node_modules` directory,
where package.json is missing or does not contain the property `"private": true`.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
where package.json is missing or does not contain the property `"private": true`.
where package.json is missing or does not contain the property `"private": true`.

Is there any chance it can be missing?

function isPrivateNodeModule(filename) {
const packageJsonReader = require('internal/modules/package_json_reader');
const resolved = StringPrototypeStartsWith(filename, 'file://') ? filename : pathToFileURL(filename).href;
const packageConfig = packageJsonReader.getPackageScopeConfig(resolved);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we cache already analized paths?
Like everything under node_modules/foo once we have already read read node_modules/foo/package.json

@jakebailey
Copy link

I said this on nodejs/typescript#14 (comment), but I don't understand the use case here; if you have a package like this, you can't have published it because it's marked private. How did you get the package, then? For monorepos, I would really expect that you'd get it via a symlink, in which case the realpath of the files will not prevent the transform from happening.

@marco-ippolito
Copy link
Member Author

I said this on nodejs/typescript#14 (comment), but I don't understand the use case here; if you have a package like this, you can't have published it because it's marked private. How did you get the package, then? For monorepos, I would really expect that you'd get it via a symlink, in which case the realpath of the files will not prevent the transform from happening.

Probably easier to use since it doesnt require additional tooling.
We could also document that possibility

@jakebailey
Copy link

That doesn't really answer my question... I understand that using this flag prevents needing another tool like tsx. But how would someone end up in the situation where their node_modules directory contains a private package, but that package is not symlinked?

@marco-ippolito
Copy link
Member Author

I thought that private: true would still allow publishing privately on the registry, which is not the case 😐
In that case yes it doesnt make sense.

@bmeck
Copy link
Member

bmeck commented Oct 15, 2024

@marco-ippolito were you thinking about bundledDependencies?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. strip-types Issues or PRs related to strip-types support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants