node-gradle / gradle-node-plugin

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

Provide parsed `package.json` contents as (or in) extension #232

Closed Vampire closed 1 year ago

Vampire commented 2 years ago

It would be nice if the plugin would parse the package.json and provide its contents readily available for further usage either as separate extension, or as field of the existing extension.

What I for example currently do is

version = file("package.json")
    .let(ObjectMapper()::readTree)
    .get("version")
    .asText()
deepy commented 2 years ago

This sounds like a good idea and should be fairly quick to implement, in the next version of the plugin I'll add a first version of this

I imagine name, version, description, homepage, license, and private might be the most immediately useful ones and might come in handy when adding additional checks Though parsing the entire file might be easy enough and maybe not everyone has ridiculously large package.json

Vampire commented 2 years ago

Having some common properties hard-coded and being able to access the rest dynamically sounds perfect. :-)

Vampire commented 2 years ago

Nice @deepy, two questions. Shouldn't node be Provider instead of Property and get and getBoolean also return Providers? And when can I expect a release with this change it you can tell?

deepy commented 2 years ago

3.3.0 was just released, so it should be possible to test right now.

Property is being used just for finalizeValueOnRead() (on 6.1 and later) to read and parse the file just once and only if it was actually requested

All the gets returning Providers makes perfect sense, looking back I'm unsure of why they aren't. I'll take another look at that after the weekend

Vampire commented 2 years ago

Ah, I see, I missed that part. I had a similar situation in the past, so I'd still like to suggest two changes.

  1. also call disallowChanges(), otherwise someone could change the value before reading it
  2. at least expose it as Provider which is a supertype of Property. One could still cast, but it makes it clearer that the property is not meant for setting a value but just to read it.

This is the pattern I used, feel free to copy or adapt it:

inline fun <reified T> cachedProvider(crossinline block: () -> T): Provider<T> =
    objects
        .property<T>()
        .value(provider { block() })
        .apply {
            disallowChanges()
            finalizeValueOnRead()
        }

val theValue = cachedProvider {
    costlyCalculateValue()
}
Vampire commented 2 years ago

And another detail, your implementation is not configuration cache safe. If you use configuration cache and for example read the version at configuration time, then edit the package.json and run again, the old configuration cache entry is reused and thus the old value for the property. I think if you use providers.fileContents as documented at https://docs.gradle.org/current/userguide/configuration_cache.html#config_cache:requirements:undeclared_file_read it should work properly.

Update: Yep, tried it with

the<PackageJsonExtension>()
    .version
    .zip(
        providers.fileContents(layout.projectDirectory.file("package.json")).asBytes
    ) { version, _ -> version }
    .get()

and then it worked properly

deepy commented 2 years ago

FileContents is introduced in 6.1 so now there's multiple compelling arguments in favour of dropping Gradle 5 support.

I'm going to check the issues and see what else should go into 4.0

Vampire commented 1 year ago

I didn't try yet, but the fileContents is most probably not necessary anymore in recent Gradle versions. The greatly improved autodetection of build configuration inputs including file reads amongst other things.

Vampire commented 1 year ago

So, I retried with Gradle 8.1 where the configuration cache became officially stable. There my fileContents-zip work-around is not necessary anymore. Gradle just determines that package.json is an input for configuration cache properly, so there is no need to change that.

One thing that could be improved though is the name of the PackageJsonExtension. Currently it is package.json which is not nicely usable. For Kotlin DSL there is no accessor generated and even in Groovy DSL it is probably not accessible conveniently due to the dot in the name. Please change the extension name to something more compatible like packageJson, then you can simply access it in the DSLs conveniently.

deepy commented 1 year ago

There's a configuration cache issue with ARM environments that needs to be solved so 4.0 will likely be only that issue + finishing up this one

deepy commented 1 year ago

4.0.0 has been released