node-gradle / gradle-node-plugin

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

Can no longer see NpmTask.result #237

Closed JamesXNelson closed 1 year ago

JamesXNelson commented 2 years ago

It used to be possible to look at the .result of an invoked NpmTask, but upon updating to latest version, I see nothing is recording the returned exec result

    @TaskAction
    fun exec() {
        val command = npmCommand.get().plus(args.get())
        val nodeExecConfiguration =
                NodeExecConfiguration(command, environment.get(), workingDir.asFile.orNull, ignoreExitValue.get(),
                        execOverrides.orNull)
        val npmExecRunner = objects.newInstance(NpmExecRunner::class.java)
        npmExecRunner.executeNpmCommand(projectHelper, nodeExtension, nodeExecConfiguration, variantComputer)
    }

please store the result object from executeNpmCommand somewhere visible to buildscripts (on the task), so smart builds that want to inspect errors and make helpful suggestions can do so.

deepy commented 2 years ago

The history of why this was removed is in https://github.com/node-gradle/gradle-node-plugin/issues/144#issuecomment-775762959

Would the proposed alternative in #144 work for your use-case? The branch is still around and I've planned to finish and merge it Soon™

JamesXNelson commented 2 years ago

As long as I can get "something that has the state about the task" after it fails, I think I can survive...

It's not 100% clear what the proposed workaround is, an API for sending hooks to later receive the result object?

I think I can survive w/ that... but, I feel like the original change "hey, lets remove the result from the task b/c stateful things are bad" is one of those "I'm making changes because I believe in a design pattern" rather than "I'm making a change that is going to improve how people can use this code".

Tasks already are stateful. The are a big bag of state, they literally have a .state field describing their current state.

JamesXNelson commented 2 years ago

Sorry for the complaining before... is there anything I can do to help make this happen?

deepy commented 2 years ago

No worries, the branch in #144 has suffered from the passage of time and doesn't merge cleanly any longer and I currently have limited spare time, I'll happily review and accept PRs. (for either solution)

The only hard requirements currently is that it works on 5.6.4 and that it doesn't break the configuration-cache.

Next version is going to be a minor but after that I might have to drop 5.6.4 from the main development.