SonarJS icon indicating copy to clipboard operation
SonarJS copied to clipboard

Yarn 2 plug'n'play and typescript

Open kirill-konshin opened this issue 5 years ago • 35 comments

Yarn has introduced a new approach to install dependencies: plug'n'play, it will be the default option for Yarn 2.

PNP means that Yarn will not copy dependencies to node_modules, it will create .pnp.js file and inject hooks into NodeJS resolution mechanisms.

Currently I Sonar Scanner seems to be unaware of that and fails with error:

ERROR: Cannot find module 'typescript'
Require stack:
- /builds/web-modules/web-modules-core/.scannerwork/.sonartmp/eslint-bridge-bundle/package/lib/tsconfig.js
- /builds/web-modules/web-modules-core/.scannerwork/.sonartmp/eslint-bridge-bundle/package/lib/server.js
- /builds/web-modules/web-modules-core/.scannerwork/.sonartmp/eslint-bridge-bundle/package/bin/server
ERROR: TypeScript dependency was not found and it is required for analysis.
ERROR: Install TypeScript in the project directory or use NODE_PATH env. variable to set TypeScript location, if it's located outside of project directory.
INFO: Sensor TypeScript analysis [javascript] (done) | time=207ms
ERROR: Missing TypeScript dependency
org.sonar.plugins.javascript.eslint.MissingTypeScriptException: Missing TypeScript dependency
	at org.sonar.plugins.javascript.eslint.EslintBridgeServerImpl.loadTsConfig(EslintBridgeServerImpl.java:270)
	at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:195)
	at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1621)
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)
	at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:913)

I also posted this to community: https://community.sonarsource.com/t/yarn-2-with-plugnplay-and-typescript/20391

kirill-konshin avatar Feb 19 '20 22:02 kirill-konshin

No need to post in two places :)

we will read about this new feature and check if it can be useful for us.

P.S. I'm not sure if you managed to succeed TS analysis, if not don't hesitate to ask for help

vilchik-elena avatar Feb 24 '20 08:02 vilchik-elena

The problem is that I haven't got response there...

I haven't managed to get it to work with PNP.

kirill-konshin avatar Feb 24 '20 17:02 kirill-konshin

ah, sorry, I misinterpreted your message (I thought you are suggesting us to rely on PNP)

Indeed we are simply launching our scripts, so they are resolving dependencies in a classic way. We will see later if we can provide a better UX, but so far I would suggest to install TS in project dir before running analysis.

vilchik-elena avatar Feb 24 '20 18:02 vilchik-elena

ah, sorry, I misinterpreted your message (I thought you are suggesting us to rely on PNP)

You NEED to rely on PNP, otherwise your package won't be able to locate TS.

Since PNP will be turned on by default in v2 you have to provide support for it... easiest way would be to use Pnpify: https://next.yarnpkg.com/advanced/pnpify

I would suggest to install TS in project dir before running analysis.

It IS installed, but using PNP. Unfortunately, since .scannerwork/.sonartmp/eslint-bridge-bundle/package/lib/tsconfig.js is launched by Java, and not within Yarn env, and it does not use .pnp.js file it cannot resolve where TS is.

kirill-konshin avatar Feb 24 '20 19:02 kirill-konshin

Hello 👋 To give some context, PnP is basically a different way to install the project dependencies. Instead of generating a node_modules directory, Yarn setups the files so that they are loaded from a common cache. This has various advantages for your customers: their projects are lighter, installs are much faster and more stable ...

Anyway, to do this, we generate a .pnp.js file which contains the list of packages and where to find them. Then when Node loads, Yarn injects this file into the runtime so that require is aware of it, and "that's it"! Almost.

Now, you may notice that I said "Yarn injects this file". This is true in most cases, because frontend developers typically run their Node scripts through yarn start or similar commands. However, in your case, it seems you're spawning the tsconfig.js process yourself, so Yarn cannot inject the PnP runtime the way it usually does. To workaround that, you need to inject it yourself by adding the following into the environment:

NODE_OPTIONS="--require path/to/.pnp.js"

This is for example what JetBrains did for their Yarn PnP support.

I hope this can help shed some light on what is PnP and what you'd need to do to support your users leveraging it. Feel free to ask me if you have any question, I designed and implemented it 🙂

arcanis avatar Feb 27 '20 19:02 arcanis

@arcanis I tried NODE_OPTIONS="--require $CI_PROJECT_DIR/.pnp.js" but it spawned next set of problems:

ERROR: /builds/web-modules/web-modules-core/.pnp.js:45217
ERROR:     throw firstError;
ERROR:     ^
ERROR: 
ERROR: Error: Something that got detected as your top-level application (because it doesn't seem to belong to any package) tried to access a package that is not declared in your dependencies
ERROR: 
ERROR: Required package: express (via "express")
ERROR: Required by: /builds/web-modules/web-modules-core/.scannerwork/css-bundle/lib/src/
ERROR: 
ERROR: Require stack:
ERROR: - /builds/web-modules/web-modules-core/.scannerwork/css-bundle/lib/src/server.js
ERROR: - /builds/web-modules/web-modules-core/.scannerwork/css-bundle/bin/server

Normally I'd fix this via packageExtensions of .yarnrc.yml, but in case of Sonar, it does not care about this :)

Can this trick be combined with pnpify w/o modifying sonar-scanner code?

kirill-konshin avatar Feb 29 '20 08:02 kirill-konshin

@vilchik-elena is there any update?

@arcanis maybe you can introduce some flag or a tool similar to pnpify that can tell Yarn to just grab any dependency from the available ones when such missed error appears? Just as an escape latch...

I tried to run NODE_OPTIONS="--require $PWD/.pnp.js" pnpify sonar-scanner but it failed with the same error. I suppose because pnpify is running in different context than scanner's sub-processes.

kirill-konshin avatar Mar 02 '20 20:03 kirill-konshin

@kirill-konshin we will investigate our options to support pnp, however it will take us some time to do it. Thanks for reporting this. In the meanwhile, as a workaround, I would suggest to install typescript in some other directory using old node_modules mechanism and use NODE_PATH so it can be found for the purpose of analysis.

saberduck avatar Mar 04 '20 09:03 saberduck

@saberduck it won't work because I have a monorepository where tsconfigs are shared between packages so in order to set things up I have to do yarn install or lerna link. So it's not just typescript that has to be installed, but all other deps too.

kirill-konshin avatar Mar 04 '20 20:03 kirill-konshin

Any updates?

kirill-konshin avatar Mar 25 '20 17:03 kirill-konshin

@kirill-konshin If there were any updates you would see them here. Please refrain from such pings, they are not helpful.

saberduck avatar Mar 26 '20 08:03 saberduck

Pardon me, but this ticket is dated Feb 19. Over a month has passed and we're still blocked, and there's no workaround.

This kind of attitude is quite surprising. I'd like to also remind about tickets like Incremental Analysis and Incremental scan in SonarQube 6.7.x are dated 2018 and have no workaround too.

Listen to your community, and please provide at least a workaround, that would be helpful.

kirill-konshin avatar Mar 26 '20 18:03 kirill-konshin

@saberduck @vilchik-elena do you have any ETA when this issue will be fixed? We can't roll out the much needed Yarn 2 w/PNP update that will speed up our builds without this SQ fix.

kirill-konshin avatar Apr 06 '20 20:04 kirill-konshin

Sorry, can't tell you anything yet. We are not working on this analyzer for the moment, we should restart in a month or so.

vilchik-elena avatar Apr 07 '20 06:04 vilchik-elena

@vilchik-elena what about any other analyzers? Do you have anything written in JS instead of Java? Any other options?

kirill-konshin avatar Apr 07 '20 08:04 kirill-konshin

@vilchik-elena @saberduck it is May 26 today, the issue was created on Feb 19. Do you have any updates?

kirill-konshin avatar May 26 '20 23:05 kirill-konshin

As an escape latch, can we disable the analysis and only upload the coverage data?

kirill-konshin avatar May 27 '20 19:05 kirill-konshin

Hi @kirill-konshin,

Sorry we can't properly investigate your problem currently. The only way to upload only coverage I see is to remove JS plugin from SQ and use generic coverage.

vilchik-elena avatar May 28 '20 08:05 vilchik-elena

Can you give me more information how to do it?

kirill-konshin avatar May 28 '20 15:05 kirill-konshin

@vilchik-elena It's been two weeks since you told that there is a way to disable the JS plugin and only use generic coverage. And almost 4 months since the issue was created. I am frustrated. I need to get this fixed one way or the other.

kirill-konshin avatar Jun 10 '20 06:06 kirill-konshin

hello @kirill-konshin, please refrain from pinging us this way. If we have time to answer this request, we will do so and you will find the information here. If there is no news, it's because we are busy with other things.

saberduck avatar Jun 11 '20 08:06 saberduck

@saberduck this is mean. You don't provide the fix. You don't provide the estimate. You don't provide the way out.

Why would I stop pinging? We're blocked for months! What am I supposed to do?

kirill-konshin avatar Jun 11 '20 17:06 kirill-konshin

@kirill-konshin how much did you pay for this product? Your tone is hardly acceptable considering that you're talking to people who provide you with high-quality software for free. Please consider that you are in no position to make demands at all. If you need this issue fixed, then either ask politely and accept if the contributors have more important things on their minds, or fix it yourself and make yourself useful to the community.

phjardas avatar Jun 11 '20 17:06 phjardas

@phjardas I am an active open source contributor, I am paying with my time in other open source projects, maybe you're even using some of them. Also I am paying with my time debugging why your product does not work with Yarn 2, trying to find solution that your community will benefit from. I have even wrote articles that promote your product. I am paying my fair share.

I was asking politely for months (!!!), yet no resolution from your side. I am happy to help, but you guys are not cooperative, let me know how can I help and I will.

Yarn 2 is stable, PNP is a standard way to load dependencies, if scanner won't support it — large portion of JS community will be left behind.

kirill-konshin avatar Jun 11 '20 17:06 kirill-konshin

Disclaimer: I am not in any way associated with SonarSource, neither am I a contributor to this project. I'm merely watching this issue because I do face the same problems as you are.

You did ask nicely in the beginning, true. In the beginning. I don't know how the people who maintain this product prioritize their work, but obviously they have decided that other things are more important. If you disagree with this decision, why don't you try to make a point and convince them that this indeed is a valuable addition to the product.

Looking at the mere stats, this issue has a single thumbs-up (from me). I'm afraid it's safe to assume that not a large portion of the JS community will be left behind. Nobody seems to really care at the moment, except for you and me. I assume there's other issues in the backlog that have much more community support.

phjardas avatar Jun 11 '20 17:06 phjardas

@kirill-konshin we are going to restart effort on SonarJS analyzer in the coming weeks (you can see some work already ongoing). We are looking into a way to support PnP, currently, we are investigating the option to package typescript within the SonarJS plugin, as it would also solve some other issues we face with current system.

I am sorry for the wait, and I understand how frustrating this can feel. I hope we will be able to unblock you soon.

saberduck avatar Jun 25 '20 15:06 saberduck

@saberduck great news, please keep this issue updated with all the findings. Looking forward to try it out.

kirill-konshin avatar Jun 29 '20 19:06 kirill-konshin

@saberduck just tried one hack, and it seemed to work: yarn unplug typescript which results in: INFO: Using TypeScript at: '/opt/wmc/.yarn/unplugged/typescript-patch-50eb28f081/node_modules'.

Unfortunately now I'm getting stuff like

23:39:24.298 DEBUG: /builds/web-modules/web-modules-core/core/src/core/ui/form/DateRangePickerWithPresets.tsx matched /builds/web-modules/web-modules-core/core/tsconfig.json

... and then

23:42:35.145 ERROR: Failed to analyze file [core/src/core/ui/form/DateRangePickerWithPresets.tsx]: File 'web-modules-build/src/typescript/tsconfig.json' not found.

The problem is that web-modules-build which is used in web-modules-core/core/tsconfig.json ("extends": "web-modules-build/src/typescript/tsconfig.json") is a monorepo package. So Sonar's TS plugin should have picked up the symlink created by Yarn.

kirill-konshin avatar Jul 08 '20 00:07 kirill-konshin

@vilchik-elena @saberduck Despite I made it work using the method described above, I've found another issue: if project is using a tsconfig that extends a config from NPM:

{
    "extends": "web-modules-build/src/typescript/tsconfig"
}

You will get an error:

ERROR: Failed to analyze file [src/modules/src/userGroups/BL.ts]: File 'web-modules-build/src/typescript/tsconfig' not found.

Since SQ is not aware were to look for it.

kirill-konshin avatar Jul 20 '20 21:07 kirill-konshin

Also some strange behavior was observed: https://github.com/SonarSource/SonarJS/issues/2040

kirill-konshin avatar Jul 20 '20 21:07 kirill-konshin