Allow NodeTask to be extended and exec to be overridden.

[pwagland]

In https://github.com/node-gradle/gradle-node-plugin/blob/main/src/main/kotlin/com/github/gradle/node/task/NodeTask.kt#L90-L99 exec is not defined as an open method, which means that tasks that extend this cannot add functionality during the execution.

In our specific case, we are using node to run SCSS, and so want to clean the target directory before calling node to populate it. I can add a second TaskAction, but there is no guarantee that this will run first, which means that sometimes I delete everything after it has been generated. Which is also not the point ;-)

I can, of course, just delegate everything, that involves a lot of boilerplate, since the there quite a few properties that should be delegated across..

So, would it be possible to make exec an open method?

[deepy]

The original reason exec() is not open is that our intention was that for these tasks you shouldn't be extending them
If you needed to extend them, you'd pick whichever Runner was most suitable and then wire that up yourself

But things changed, designs changed, and setting the wiring up might be the harder part of this

We could consider changing this, but could you create an example project showing the issue?

In the short-term I think either depending on a Delete task or making use of doFirst() might work

[pwagland]

Hi @deepy,

Thanks for your comment, while I can't extract out everything, I can give a taste of what we are trying to do:

We have an ScssTask:

@CacheableTask
abstract class ScssTask extends NodeTask {
  @Input
  public List<String> getArguments() {
    var result = new ArrayList()
    result.add("--source-map")
    result.add("--style=compressed")
    argsList.each(v -> result.add(v instanceof Property ? v.get().toString() : v.toString()))
    result.add(inDir.getOrElse("").toString() + ":" + outDir.getOrElse("").toString())
    (project.configurations.uiScss + project.configurations.uiScssIn + project.configurations.uiScssIn.artifacts.files).each {
      var dir = it
      result.add("--load-path=${dir}")
    }
    result.each { logger.debug("--DEBUG: ${getName()} ${getClass()} argument list : " + it) }
    return result
  }

  @InputFiles
  @PathSensitive(PathSensitivity.RELATIVE)
  public Collection<File> getInputFiles() {
    // This code is calculating and returning all empty directories from sourceSet excluding src/
    // all subdirectories of src containing *.scss files
    HashSet<File> inputFiles = new HashSet<>()
    inputFiles = Util.findDirsIncluding(project, project.configurations.uiScssIn, ['**/*.scss'])
    inputFiles.addAll(project.configurations.uiScss.files)
    inputFiles.addAll(project.configurations.uiScssIn.artifacts.files)
    return inputFiles
  }
}

Now what we have discovered is that if the input files have changed, sometimes we need to remove the entire output, as otherwise we might end up with old, and now incorrect, files in our cached build results, so this is where we want to

We cannot use doFirst(), as it isn't guaranteed to run before our action. Related reading:

• https://issues.gradle.org/browse/GRADLE-2064
• https://discuss.gradle.org/t/dofirst-does-not-execute-first-but-only-after-task-execution/28129

The workaround that we have adopted is to add the clean task as a dependency, since it is cached, rebuilding doesn't take long, but it seems wasteful to always delete, and then unpack the cache. If we could override exec then we could just add:

  @Override
  @TaskAction
  public void exec() {
    clean();
    super.exec();
  }

to ensure that the output was always cleaned, and would therefore always be correct.

[deepy]

NodeTask is really just a thin wrapper around the NodeExecRunner
I think based on looking at the code that the best way would be copying the source code for the entire NodeTask, removing the bits you don't need and replacing exec with what you want

But while that makes perfect sense to me, it falls apart the moment you want to do something like:

tasks.withType<NodeTask>().configureEach {
  environment.put("something")
}

And while you maybe could work around that by having two tasks and doing something convoluted with finalizedBy I'm already starting regretting trying to model that flow in my head

The only reason I'm reluctant is because even though the NodeTask isn't very opinionated it's starting to get difficult to maintain the plugin as a whole, and having "control" over the exec method makes it very easy to change things
But with Gradle 9 and my vacation looming over me, maybe it's time to redesign that API

[pwagland]

Hi @deepy,

Thanks for your continued consideration.

FWIW, it turns out that doFirst does seem to work correctly now? At least, in our case we are using it, and no-one seems to be complaining. So I guess the ordering issue was fixed at some point in gradle the last 8 years?

[deepy]

If it works then I'm happy not to investigate further, but skimming the issues again it looks like this might've affected us previously since NodeTask used to be an Exec task
But now that we're invoking exec manually those limitations don't apply