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

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

Closed
Vampire opened this issue Apr 14, 2022 · 11 comments · Fixed by #236
Closed

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

Vampire opened this issue Apr 14, 2022 · 11 comments · Fixed by #236
Milestone

Comments

@Vampire
Copy link

Vampire commented Apr 14, 2022

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
Copy link
Member

deepy commented Apr 21, 2022

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
Copy link
Author

Vampire commented Apr 26, 2022

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

deepy added a commit that referenced this issue May 2, 2022
@deepy deepy linked a pull request May 4, 2022 that will close this issue
@deepy deepy added this to the 3.3 milestone May 5, 2022
deepy added a commit that referenced this issue May 14, 2022
@Vampire
Copy link
Author

Vampire commented May 14, 2022

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
Copy link
Member

deepy commented May 14, 2022

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
Copy link
Author

Vampire commented May 15, 2022

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
Copy link
Author

Vampire commented May 16, 2022

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 deepy reopened this May 16, 2022
@deepy
Copy link
Member

deepy commented May 16, 2022

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

@deepy deepy modified the milestones: 3.3, 4.0 May 16, 2022
@Vampire
Copy link
Author

Vampire commented Apr 17, 2023

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
Copy link
Author

Vampire commented Apr 18, 2023

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
Copy link
Member

deepy commented Apr 20, 2023

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 deepy modified the milestones: 4.1, 4.0 Apr 20, 2023
@deepy
Copy link
Member

deepy commented Apr 23, 2023

4.0.0 has been released

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

Successfully merging a pull request may close this issue.

2 participants