Fix #967 : Add pnpm executable to PATH

[JulesAaelio]

Summary

Fix the issue described in #967

the PNPMInstaller tries to copy pnpm & pnpm.cmd (copyPnpmScripts), but pnpm has no such predefined files, thus it does not find anything to copy.

The solution I propose is to create a symlink to node_modules/pnpm/bin/pnpm.cjs when no pnpm or pnpm.cmd is available.

Tests and Documentation

I tested using this dummy project : https://github.com/JulesAaelio/test-frontend-maven-plugin

[eirslett]

How will this work on Windows (which doesn't have symlinks, I think)?

[GuillaumeC]

pnpm work with symlinks to a global store. So it should be work on Windows too.

[JulesAaelio]

Hi @eirslett ,
You have a valid point here, although symlinks are supposed to work on Windows 10 it is still unclear under what conditions.

According to their documentation, pnpm does'nt use symlinks neither, but junctions, that we cannot use here because we're trying to create a link to a file and not to a directory. https://pnpm.io/faq#does-it-work-on-windows

The others options to make this works on windows are :
As stated in the issue :

to execute cmd-shim if the files are not found.
to install the .tgz files with npm instead of just extracting them, which I assume would add the executables automatically. But of course that would require the npm to be installed in advance.

Or :
to use the freshly installed pnpm to "install" itself so it creates the executables.

=> As all of these are non-trivial, I propose to simply add a check before creating the symlink to ensure that we're running on a non-windows platform. I updated my code accordingly .

[eirslett]

👍

However, this project has an integration test that tries to run pnpm. As far as I can see, that integration test is now passing. Is it possible to create a reproduction of this bug/issue (a failing integration test), to show that the problem has indeed been fixed with this patch?

[JulesAaelio]

Sure, I can have a look. Should I include the new test in this merge request or in a new one ?
EDIT: Added in #1053

[eirslett]

@JulesAaelio I merged your PR with the failing integration test. As you can see here: https://github.com/eirslett/frontend-maven-plugin/actions/runs/3169669688/jobs/5161739948 it looks like the integration test is still failing on Ubuntu? Can you have a look?

If you rebase your pull request branch, the integration test should be included in there, so it's easier to check whether the test passes or fails.

[JulesAaelio]

Hi @eirslett
Looks like the integration tests on ubuntu were failing due to a version mismatch between pnpm and node (Seems like pnpm compatibility table is not up to date).

=> I bumped the versions and the tests are now behaving as expected :
Failing on windows and passing on ubuntu
The macos job was cancelled because the windows job failed

[eirslett]

👍 So how do we make the test pass on Windows then?

[JulesAaelio]

As I said on my previous comment, making it work on windows is a bit more complicated and I don't feel like digging into it.
So I used a profile to ensure the test doesn't run.
It is not really a fix but the tests are passing. Let me know what you think :)

[eirslett]

Ok, let's merge it anyways! And hopefully somebody on Windows will fix it if they need it.