node-gradle / gradle-node-plugin

Gradle plugin for integrating NodeJS in your build. :rocket:
Apache License 2.0
599 stars 117 forks source link

Update NodePlugin.kt #278

Open donarus opened 1 year ago

donarus commented 1 year ago

The addPnpmRule should use the same logic as addNpmRule.

It should create (and not only register) the task.

donarus commented 1 year ago

the reason for this is that right now the task is not created until task collection is asked to create it, because register is lazy by default.

Right now, one have to overcome this issue by calling the "get" method on TaskProvider to create the real task and then it can be used in depends_on

tasks.named("pnpm_run_build").get() // see

tasks.register<DefaultTask>("build") {
    dependsOn("pnpm_run_build")
}
deepy commented 1 year ago

On one hand this makes perfect sense and this absolutely shouldn't differ between the different tools But on the other hand, we want to discourage the use of the rule-generated tasks

The reasoning behind this is that the moment you want to depend on something then this is no longer an ad-hoc task being run, and ad-hoc tasks is what the rule is meant for If you've got something that's being run often, like say foo_run_build ideally you should create a proper task with inputs/outputs and all that jazz and name it fooRunBuild

I need to do some quick searches and see how these are being used in the wild, I can see why this is convenient and it might be that it should just be solved by proper documentation rather than artificial technical limitations

deepy commented 1 year ago

Oh and could you add a test case to https://github.com/node-gradle/gradle-node-plugin/blob/master/src/test/groovy/com/github/gradle/node/pnpm/task/PnpmRule_integTest.groovy showing that it works?

The way the tests work is by copying one or two base setups from https://github.com/node-gradle/gradle-node-plugin/tree/master/src/test/resources/fixtures which contains general project setup and then either adding to or writing a fresh build.gradle

I think copying the pnpm_install test in there should work fine as a base for this test