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

Update NodePlugin.kt #278

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

donarus
Copy link

@donarus donarus commented Jun 5, 2023

The addPnpmRule should use the same logic as addNpmRule.

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

The addPnpmRule should use the same logic as addNpmRule.

It should create (and not only register) the task.
@donarus
Copy link
Author

donarus commented Jun 5, 2023

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
Copy link
Member

deepy commented Jun 7, 2023

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
Copy link
Member

deepy commented Jun 7, 2023

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

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 this pull request may close these issues.

2 participants