node-gradle / gradle-node-plugin

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

Allow NodeTask to be extended and exec to be overridden. #307

Open pwagland opened 5 months ago

pwagland commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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:

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.