#67 support pnpm

[langrp]

I'd like to support pnpm similar way as npm, without need of npx.

[deepy]

Unfortunately I can't review this right now, but is there anything in this that differs from the npm implementation?

I'm mostly curious because we might want to rethink how we do this in the future, especially if nothing is different (apart from the executable name)

[langrp]

I didn't want to differ from npm nor yarn so it's very similar. It uses npm to download required version of pnpm, if not defined it downloads the latest version. It also uses local pnpm if available in node_modules. The tasks defines inputs and outputs.

[langrp]

I mean the implementation is as similar as npm and yarn implementation. And you're right, there could be more generic implementation of such executables.

[bsautel]

I was also wondering whether we wouldn't have better to reuse the existing code for npm and simply add a configuration property to tell that we want to use pnpm and pnpx instead of npm and npx.
It sounds like pnpm's CLI commands are not the same as npm's ones, but we could probably adapt the code according to the package manager which is used.

Is it better than writing another implementation like you did? For now, I don't know.

But if we use a more generic approach, it would probably make sense to do the same thing for yarn. But this would clearly break backwards compatibility.

[langrp]

Thanks guys for quick comments. I thought of reusing npm a lot. But the pnpm is more like yarn in regards to the plugin, despite of its similar name. It needs to get downloaded (uses npm to do so). All paths are different as well, e.g. npm uses npm-cli.js while pnpm.js. The inputs/outputs differs on files and version parameter, pnpm generates pnpm-lock.yaml. If yarn has a place for own implementation then pnpm should get as well.

Having npm implementation to take parameters to fulfil pnpm would mean to consider yarn the same way.
Therefore generic approach would be great. Of course backward compatibility could be achieved on task level to keep respective classes for gradle build scripts.

What I think is build script like

node {
    version = '10.6.0'
    packager {
        pnpm {
            version = '4.12.4'
            workDir = project.file('.gradle/pnpm')
        }
    }
}

where packager block would define only single packager manager as only one would be used in projects anyway. Such definition could create task pnpmTask and pnpmSetup. For backwardd compatibility the old parameters would be kept as well, but deprecated.
This approach shall allow custom manager definition

node {
    version = '10.6.0'
    packager {
        custom {
            command = 'my_npm'
            version = '4.12.4'
            workDir = project.file('.gradle/pnpm')
            script = 'my_npm/bin/my_npm'
        }
    }
}

[bsautel]

Sorry for my late answer, I had to take some time to think about that. I was working on a big refactoring aiming at supporting correctly Gradle lazy configuration that I pushed it #93. This gave me the opportunity to think about that.

I think that your suggestion (regarding the packager to use and ho we configure it) is relevant. But this needs yet another big refactoring that will take much time.

I suggest we proceed this way:

1. We integrate now pnpm support as you suggested in the pull request, the same way as yarn is supported.
2. In a later 3.x version, we will do this refactoring to introduce the concept of packager. We will ensure it is backward compatible so that the 3.0 configuration still works.
3. We will drop backward compatibility of this packager mechanism in version 4.0.

This pull request will require some additional work to get it mergeable into the master branch (mostly when we merge #93 which changed many internal thinks). It is not currently merged since I would like a review.

@langrp would you be volunteer to adapt your pull request in order to follow #93 changes?
To sum up quickly:

• ExecRunner changed a lot
• Variant no longer exists
• All the configuration fields are now Gradle properties to fully support lazy configuration
• New Kotlin DSL test that ensures all configuration and tasks properties can be used with the Kotlin DSL

This implies some important changes in the production code but should change nearly nothing in the tests since those changes are backward compatible in the Groovy DSL point of view.

[langrp]

Good news :).
I'll update my changes on top of #93 changes

[bsautel]

Great!

[bsautel]

@langrp , could you update your branch from the master branch?

I'll have a look at why the tests are broken, but it will be easier if I only see the diff from the latest master version.

[langrp]

@bsautel I have moved changes on top of master. Strangely the test fails on windows only, which I can't test locally.

[bsautel]

Thanks @langrp for these changes.

I added a few comments to try to help you fix the broken tests on Windows. That's not always easy!

[langrp]

@bsautel I had to ignore PNPX tests on windows as there seems to be an issue pnpm/pnpm#948. Other tests are fixed

[bdellegrazie]

Any chance of getting this merged?

[deepy]

Now that 3.0.0 has stabilised it's probably a good time to have another look at this, currently it doesn't compile if merged into master, it breaks on Unresolved reference: computeNpmProxyCliArgs at PnpmSetupTask.kt and PnpmExecRunner.kt (which is understandable as we reworked the proxy support)

[bhagyas]

Is there any updated information on this?

[deepy]

Currently we have proxy tests that suddenly started failing, that's a big blocker and currently the highest priority issue, you can see the current priority in the upcoming milestone

The problem with the tests is that I can't just chip away at it like usual and in the past month I've had limited availability.

[bhagyas]

Currently we have proxy tests that suddenly started failing, that's a big blocker and currently the highest priority issue, you can see the current priority in the upcoming milestone

The problem with the tests is that I can't just chip away at it like usual and in the past month I've had limited availability.

Thanks for the update. I was able to get pnpm to work using NpxTask for the moment.

[DreierF]

It would be really nice to have pnpm support in the the plugin.

@deepy Is there anything we could do to push this PR over the finish line? Are there still any blockers besides the merge conflicts?

[deepy]

merge conflicts and test failures, if you can revitalize the PR I promise to take care and merge it before starting anything else

[deepy]

@DreierF ideally by checking out the branch and merging master into it, creating a second PR from that branch

[DreierF]

@deepy Thanks! I have created a new PR #240