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

Cannot find module 'ts-node' (yarn) #636

Closed
ntucker opened this issue Apr 3, 2023 · 34 comments · Fixed by #639
Closed

Cannot find module 'ts-node' (yarn) #636

ntucker opened this issue Apr 3, 2023 · 34 comments · Fixed by #639

Comments

@ntucker
Copy link
Contributor

ntucker commented Apr 3, 2023

Bug report

Actual Behavior

Attempts to build using this loader result in the following:

! Module build failed: Error: Cannot find module 'ts-node'
ERR! Require stack:
ERR! - /home/circleci/project/node_modules/cosmiconfig-typescript-loader/dist/cjs/index.js

Expected Behavior

https://github.com/Codex-/cosmiconfig-typescript-loader/blob/main/package.json#LL39C6-L39C13

cosmiconfig-typescript-loader has a peerDep of ts-node. This module has cosmiconfig-typescript-loader as a dependency - therefore it should specify ts-node as a dependency as well.

How Do We Reproduce?

ntucker/anansi#1941

https://app.circleci.com/pipelines/github/ntucker/anansi/9024/workflows/561c1fc3-70c1-403c-a033-8175c9788786/jobs/47985

Please paste the results of npx webpack info here, and mention other relevant information

  System:
    OS: Linux 5.15 Ubuntu 20.04 LTS (Focal Fossa)
    CPU: (20) x64 AMD Ryzen 9 7950X 16-Core Processor
    Memory: 27.62 GB / 31.21 GB
  Binaries:
    Node: 18.15.0 - ~/.nvm/versions/node/v18.15.0/bin/node
    Yarn: 3.5.0 - ~/.yarn/bin/yarn
    npm: 9.5.0 - ~/.nvm/versions/node/v18.15.0/bin/npm
  Monorepos:
    Yarn Workspaces: 3.5.0
  Packages:
    babel-loader: 9.1.2 => 9.1.2 
    webpack: 5.77.0 => 5.77.0 
    webpack-cli: 5.0.1 => 5.0.1 
    webpack-dev-server: 4.13.2 => 4.13.2 
@alexander-akait
Copy link
Member

@ntucker
Copy link
Contributor Author

ntucker commented Apr 3, 2023

@alexander-akait 7.2.1 is the version this broke in. Previous versions work fine. You can even see that in 7.1 the problem package cosmiconfig-typescript-loader is not included https://github.com/webpack-contrib/postcss-loader/blob/v7.1.0/package.json#L44

@alexander-akait
Copy link
Member

I can't reproduce, check your version again

@alexander-akait
Copy link
Member

@alexander-akait alexander-akait unpinned this issue Apr 3, 2023
@ntucker
Copy link
Contributor Author

ntucker commented Apr 3, 2023

See https://yarnpkg.com/features/pnp if you don't specify the dependency, yarn will not find it because its resolution is ambiguous. If you want this to be optional you must specify it as an optional peerdependency

https://github.com/webpack-contrib/postcss-loader/blob/master/src/utils.js#L56 this line should not be executed unless the dependency is listed in the package executing it. Simply adding this as a peerDep solves this problem.

I'm happy to make a PR if you're unsure how to do this.

@alexander-akait
Copy link
Member

Sorry I don't undestand you, we don't use ts-node and other things directly? Why we should add them?

@ntucker
Copy link
Contributor Author

ntucker commented Apr 3, 2023

  • you use it right here
  • ts-node is listed as a peerDependency of cosmiconfig-typescript-loader, therefore you must add it so it can be resolved correctly

Even in loose mode of yarn it will warn you about these bugs

@alexander-akait
Copy link
Member

alexander-akait commented Apr 3, 2023

PR welcome, not sure how we can fix it here, because it is an optinal dependecy, so we can't list it in dependecies

@alexander-akait alexander-akait changed the title Cannot find module 'ts-node' Cannot find module 'ts-node' (yarn) Apr 3, 2023
@alexander-akait
Copy link
Member

@alexander-akait
Copy link
Member

alexander-akait commented Apr 3, 2023

Only one thing we can do - inline code of the typescript cosmiconfig loader and put everything in peer deps

@ntucker
Copy link
Contributor Author

ntucker commented Apr 3, 2023

Only one thing we can do - inline code of the typescript cosmiconfig loader and put everything in peer deps

I don't think that's necessary, at least with yarn #639 works. I believe this works for pnp as well.

@alexander-akait
Copy link
Member

Not sure about typescript... Somebody can test it on yarn pnp?

@ntucker
Copy link
Contributor Author

ntucker commented Apr 3, 2023

It lists it as a peerDep. if it isn't used (require/imported) in the package they should remove it

@alexander-akait
Copy link
Member

That is why I said typescript should not be listed, because we don't use it

@akphi
Copy link

akphi commented Apr 3, 2023

I can't upgrade to 7.2.1 neither due to this error, found it in our webpack build. Also, we never use ts-node and I can't find ts-node installed by any packages in our yarn.lock so obviously we would bump into this Error: Cannot find module 'ts-node' problem.

As @ntucker has mentioned,

if this require fails, why isn't it silently caught by the try ... catch ... but bubble up all the way and cause webpack to fail?

@alexander-akait
Copy link
Member

alexander-akait commented Apr 3, 2023

@akphi Because it is yarn and pnp with some magic logic

@joan38
Copy link

joan38 commented Apr 3, 2023

Getting hit by the same issue here.

Why npm projects keep breaking without any changes even if we set versions in stone? Everytime it's the same thing! We all are busy with other fish to fry, we cannot bother with this now.

It's like if you have to go to an appointment with your car and suddenly you're car is not working. Now you have to debug and update the car. NO, I need the car now...

Sorry for ranting but this is super frustrating...

@jlvandenhout
Copy link

jlvandenhout commented Apr 3, 2023

They list typescript as a peer dependency because ts-node lists it as a peer dependency here: https://github.com/TypeStrong/ts-node/blob/main/package.json#L145

@alexander-akait Doesn't matter if you use ts-node or typescript in this package, you need to either list it as dependency here to fulfill the peer dependency requirements, or list them as peer dependencies to let other packages up the dependency tree fulfill the requirements.

@ntucker
Copy link
Contributor Author

ntucker commented Apr 3, 2023

@ntucker They list typescript as a peer dependency because ts-node lists it as a peer dependency here: https://github.com/TypeStrong/ts-node/blob/main/package.json#L145

Doesn't matter if you use ts-node or typescript in this package, you need to either list it as dependency here to fulfill the peer dependency requirements, or list them as peer dependencies to let other packages up the dependency tree fulfill the requirements.

Yes, that's why I included it in my PR https://github.com/webpack-contrib/postcss-loader/pull/639/files. You should be telling @alexander-akait this since he was the one asking about why typescript was needed.

@ntucker
Copy link
Contributor Author

ntucker commented Apr 3, 2023

Getting hit by the same issue here.

Why npm projects keep breaking without any changes even if we set versions in stone? Everytime it's the same thing! We all are busy with other fish to fry, we cannot bother with this now.

It's like if you have to go to an appointment with your car and suddenly you're car is not working. Now you have to debug and update the car. NO, I need the car now...

Sorry for ranting but this is super frustrating...

What we need is a github action that let's library authors verify compatibility easily so random libraries don't accidentally violate these things. Even without those tools not specifying this properly leads to ambiguous resolution, which can lead to incredibly difficult to track down issues. It would have been better if these rules were in place at the inception but that's a lot of ask of new tools

@alexander-akait
Copy link
Member

https://github.com/webpack-contrib/postcss-loader/releases/tag/v7.2.2

@jlvandenhout
Copy link

This still breaks for us. I think because cosmiconfig-typescript-loader is imported without check for ts-node here, which imports ts-node here. So probably the import of cosmiconfig-typescript-loader should become a dynamic import in this block right? wdyt @ntucker?

@alexander-akait
Copy link
Member

And I said it above that it doesn't help, but for some reason no one listened to me

@akphi
Copy link

akphi commented Apr 3, 2023

Yep, I don't think it worked for me, after all, ts-node is not available in my node_modules, so the only workaround I can follow right now is to install ts-node somewhere 😢

@alexander-akait
Copy link
Member

The main reason this problem is https://github.com/Codex-/cosmiconfig-typescript-loader/blob/main/package.json#L36:

  • "@types/node" why they there?
  • "typescript" should be not be there, it is an optinal dependecy (must be) of ts-node, for example you can use swc and don't have typescript, and ts-node will work

The latest point for us the same, that is why typescript should not be in our dependecies

Ideally cosmiconfig-typescript-loader should have only one optinal peer dependecy - ts-node, even "cosmiconfig" is extra, because I can reuse a logic in other config loaders, like lil-config and etc.

@alexander-akait
Copy link
Member

The latest point for us the same, that is why typescript should not be in our dependecies

And yes, I still thinking it should be remove from our peer deps, because I can realy use swc or something else

@alexander-akait
Copy link
Member

https://github.com/webpack-contrib/postcss-loader/releases/tag/v7.2.3

@ntucker
Copy link
Contributor Author

ntucker commented Apr 4, 2023

The main reason this problem is https://github.com/Codex-/cosmiconfig-typescript-loader/blob/main/package.json#L36:

* `"@types/node"` why they there?

I suspect this may be because they refer to these types with some of their types. This is less likely to break as typescript has more gracefully fallbacks so I didn't bother looking up if it was necessary and just skipped it in my earlier PR.

* `"typescript"` should be not be there, it is an optinal dependecy (**must be**) of `ts-node`, for example you can use `swc` and don't have `typescript`, and `ts-node` will work

https://www.npmjs.com/package/ts-node?activeTab=code ts-node does not list typescript as optional. only @swc/core and @swc/wasm are (this is the peerDependenciesMeta section)

The latest point for us the same, that is why typescript should not be in our dependecies

No, ts-node definitely requires typescript. They don't list it as optional. Here is one of their imports https://github.com/TypeStrong/ts-node/blob/main/src/configuration.ts#L2 so it was not a mistake on their part. This makes sense because ts-node is a TypeScript utility - "TypeScript execution and REPL for node.js, with source map and native ESM support." peerDependencies allow packages to utilize the correct version others choose for their package. In this case, it is TypeScript. If you're going to make any package have some interface with TypeScript, you'll need it in your peerDeps. If that functionality is optional then you can make the peer dependency optional. However, having it is absolutely necessary.

Furthermore, adding extra peerDeps that aren't even used do not break the package itself or cause it to throw runtime errors - they will simply break during the install phase (never at runtime). Depending on the package manager this might be a warning, an error or they might just ignore the requirements altogether. In no case will it cause runtime errors.

Ideally cosmiconfig-typescript-loader should have only one optinal peer dependecy - ts-node, even "cosmiconfig" is extra, because I can reuse a logic in other config loaders, like lil-config and etc.

As explained above, its peerdeps are specified correctly as far as typescript is concerned. For @types/node I didn't bother verifying because I was lazy and typescript doesn't fail as hard when the types are missing.

@ntucker
Copy link
Contributor Author

ntucker commented Apr 4, 2023

The latest point for us the same, that is why typescript should not be in our dependecies

And yes, I still thinking it should be remove from our peer deps, because I can realy use swc or something else

Just because it works in your case with a less strict manager, that doesn't mean it will break if specify it correctly, nor that it will work with more strict package managers. Additionally, as I mentioned previously the reason they started putting in these stricter rules is because there are a bunch of edge cases that break with packages if these aren't specified correctly. These can still happen in more forgiving tools like swc. The difference is the stricter tools tell you much earlyer (fail early, fail often) rather than waiting of some untraceable confusing error that makes no sense.

@ntucker
Copy link
Contributor Author

ntucker commented Apr 4, 2023

This still breaks for us. I think because cosmiconfig-typescript-loader is imported without check for ts-node here, which imports ts-node here. So probably the import of cosmiconfig-typescript-loader should become a dynamic import in this block right? wdyt @ntucker?

Yup, that's on point. The previous PR only fixed the case for those who are wanting to use it with TypeScript. Since it's optional, the case without typescript must be handled correctly as well.

@alexander-akait
Copy link
Member

No, ts-node definitely requires typescript. They don't list it as optional.

That is weird, because if I wanted to avoid using typescript directly (for example for CI/CD and use only swc) and reduce my size of node_modules, I will always have typescript and spend time on it, I feel like it's not right at all.

Furthermore, adding extra peerDeps that aren't even used do not break the package itself or cause it to throw runtime errors - they will simply break during the install phase (never at runtime). Depending on the package manager this might be a warning, an error or they might just ignore the requirements altogether. In no case will it cause runtime errors.

Using this logic, webpack has terser-webpack-plugin in deps, but terser-webpack-plugin has swc/esbuild/uglify/terser/etc packages in peer deps, should we add them to peer deps? In the end, we will only add all the time so that suddenly something does not break, this is fundamentally bad idea.

We faced with these bugs because our dependecies has wrong logic for peer deps and now we should do the same, now everyone who uses sass-loader, for example various boilerplaters should do the same using your logic...

@alexander-akait
Copy link
Member

Please update - https://github.com/webpack-contrib/postcss-loader/releases/tag/v7.2.4, to avoid memory leak

@ntucker
Copy link
Contributor Author

ntucker commented Apr 4, 2023

terser-webpack-plugin peerDeps is webpack only. it is a plugin for webpack so this makes sense it needs to interface with the users existing webpack.

If you don't use peerDeps you actually end up with multiple copies of packages which bloats your node_modules size more.

I'm not sure how swc works, but newer versions of npm will just install peerDeps even if you don't list them in your project. So you're ending up with typescript somewhere but it might not be resolved correctly. Why do you think you can execute typescript without typescript?

If you're having troubles with node_modules size, you should probably use some pnp variant as the majority of the performance issues are related to the numerous files (tho it does lower the size as well). But then if you do that you'll definitely want those peerDeps specified correctly.

https://yarnpkg.com/features/pnp
https://pnpm.io/benchmarks

@alexander-akait
Copy link
Member

If you don't use peerDeps you actually end up with multiple copies of packages which bloats your node_modules size more.

I'm afraid you don't understand me

I'm not sure how swc works, but newer versions of npm will just install peerDeps even if you don't list them in your project.

I am about unnecessary peer dependecies and don't make them optionally

If you're having troubles with node_modules size, you should probably use some pnp variant as the majority of the performance issues are related to the numerous files (tho it does lower the size as well).

Sorry, I fully disagree with this, again - I don't need typescript, why ts-node ask me to install it (i.e. why it is in peer deps), ts-node can work without typescript, so why is the matter? Why I need to install unnecessary packages? Spend time and space. I don’t want to look for a way to reduce what I don’t need at all

My question is about why developers add such more things in them peer deps when it can be less and many of them can be optional.

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

Successfully merging a pull request may close this issue.

5 participants