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

Allow plugin to be configured without adding gradle task rules by default #116

Closed
nhoughto opened this issue Sep 28, 2020 · 8 comments
Closed
Milestone

Comments

@nhoughto
Copy link
Contributor

Happy to raise a PR for this one if interested.

The Plugin as it is described today, (both 2.x and 3.x) automatically registers Gradle Task Rules to automatically pick up yarn_x etc without having to be created automatically, whilst this is useful to get up and running quickly it can create a situation where tasks exist and can be called that aren't defined in build.gradle (duh). But this can cause problems where you are explicitly configuring tasks with dependencies / behaviour so that they work within a large system, and someone comes along and uses a new yarn_run.xxx command that appears to be supported but is in fact not.

It would be better / more obvious if only the commands that were supported and configured worked, whilst all other commands would fail with 'tast not found' as you would normally expect in Gradle.

The change for this would likely be a second Plugin class that is the same as the current one, but without the addRule() calls, a consumer of the plugin could opt-into this new behaviour by applying this new plugin class, instead of the original?

@deepy
Copy link
Member

deepy commented Sep 28, 2020

It'd probably be easier to just extract the addRule() calls to a second plugin, have the second plugin apply the first plugin and then add the rule.

Though from memory I think it might be possible to add a setting on the NodeExtension and exposing it as a setting, which I think would be the preferred way if it works with all the cases.

The way we do this where I work is that we only use camel-case tasks, e.g. npmRunDist rather than npm_run_dist, but we don't really have people exploring for additional undocumented tasks.

@bsautel
Copy link
Contributor

bsautel commented Sep 28, 2020

Thanks @nhoughto for this proposal.

Personally, I was thinking recently to this task rules feature and my conclusion is that I would be in favor to deprecate this feature. The main reason is that those virtual tasks cannot be configured at all (dependencies / inputs / outputs).

Gradle's up-to-date checking mechanism is very powerful and makes the build very fast when there is nothing to do. When you use Node.js in a Gradle build, it is often to build a web application and those tasks generally take a lot of time (npm install, build the webapp, run the tests). If you misconfigured the inputs/outputs, all the builds will take a lot of time, even when those tasks don't need to run. We can see an example here. Configuring correctly the inputs and outputs of tasks requires some Gradle knowledge, but it's necessary to have a fast build.

What do you think about that?

Personally I don't use task rules. Maybe there are some use cases where it is useful and where these limitations I am talking about are not a big deal?

@deepy
Copy link
Member

deepy commented Sep 28, 2020

I don't have any strong opinions regarding this feature, as I don't use them either, but I believe in our case they are useful when you need to run an ad-hoc task or a command that maybe doesn't tie in with the result of your build. (e.g. npm_serve to start a dev-server).

You can however access and configure these tasks just like usual, so something like this would be perfectly logical and acceptable:

tasks.named('npm_hello').configure { outputs.file('hello.txt') }

Except the moment you're configuring these automatic tasks you're at the point where it'd probably make more sense to create them and have them being first-class citizens of your build-script instead of relying them hopefully being there and hopefully being configured like you want.

With that said, maybe only the npm_run_ rule is useful in reality, I don't work enough in node to give a good answer.

@nhoughto
Copy link
Contributor Author

Yeah the adhoc tasks are only helpful for the very simple cases, you pretty quickly get past those limitations tho, a yarn run build --env=production type of thing is pretty common and not supported (i dont think?) without further task config.

@nhoughto
Copy link
Contributor Author

Seems like should merge it into both 2.x and 3x branches, since 3.x isn't even GA yet.. i'd start with 2.x? What is the tip of that branch to PR instead? Is it 2.x branch?

@nhoughto
Copy link
Contributor Author

Raised against 2.x branch, #117

Unsure about move the addRule() call to Project.afterEval.. wonder if it changes the contract/behaviour to plugin users?

@nhoughto
Copy link
Contributor Author

Ended up just fixing this after plugin apply via reflection, not my favourite but better than maintaining a fork!

def removeNodeTaskRules(Project thisProject) {
    //because https://github.com/node-gradle/gradle-node-plugin/issues/116
    Field rulesField = thisProject.tasks.class.getSuperclass().getSuperclass().getSuperclass().getSuperclass().getDeclaredField("rules")
    rulesField.setAccessible(true)
    List<Rule> taskRules = rulesField.get(thisProject.tasks)
    def rulesToRemove = taskRules.findAll { it.description.contains("npm_") || it.description.contains("yarn_")}
    if (rulesToRemove != null) {
        taskRules.removeAll(rulesToRemove)
    }
}

@deepy deepy added this to the 3.3 milestone Oct 11, 2021
deepy added a commit that referenced this issue Apr 18, 2023
@deepy
Copy link
Member

deepy commented Apr 20, 2023

This is now possible in 3.6.0

node {
    enableTaskRules = false
}

@deepy deepy closed this as completed Apr 20, 2023
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

No branches or pull requests

3 participants