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

Add source generator that reads a JSON value as a const string at compile time. (OSOE-819) #238

Closed
sarahelsaig opened this issue Feb 21, 2024 · 10 comments · Fixed by #245, #250, #251 or #260
Closed
Assignees

Comments

@sarahelsaig
Copy link
Member

sarahelsaig commented Feb 21, 2024

Disclaimer: I'm not sure if this is the right repo for this suggestion. I don't think we have done any source generators yet, so maybe it should be a new repo if we do this. The idea ties into node/npm but it's not dependent on Node Extensions.

Problem

When you include a vendor resource in OC, it's a good idea to include the version too, for the purpose of cache busting. For example

        _manifest
            .DefineScript(Library)
            .SetUrl(Vendors + "chart.js/Chart.min.js", Vendors + "chart.js/Chart.js")
            .SetVersion("2.9.4");

As you can see, the version here is just a raw string. It's derived from the matching "chart.js": "2.9.4", in package.json. Then later you update the package file and forget to update the matching version number in the resource manifest! It's a maintenance nightmare.

Solution

Create a source generator that creates a compile time constant. For instance:

[ConstantFromJson("ChartJsVersion", "package.json", "$..['chart.js']")]
public class ResourceManagementOptionsConfiguration : IConfigureOptions<ResourceManagementOptions>
{
    // ...
        _manifest
            .DefineScript(Library)
            .SetUrl(Vendors + "chart.js/Chart.min.js", Vendors + "chart.js/Chart.js")
            .SetVersion(ChartJsVersion);
    // ...
}

Here [ConstantFromJson(variableName, fileName, jsonPath)] would instruct the source generator to insert string constant called variableName and set its value to the first result in fileName that matches jsonPath. Then at compile time it will become private const ChartJsVersion = "2.9.4";. Of course, there should be a shortcut like [ValueFromNpm("chart.js")] as well.

I looked for existing projects, but this was the only thing vaguely similar.

Alternatives

You could mark the package.json an embedded file, then read & parse it rune time at every startup.
Just writing that sentence down made me feel ill.

Jira issue

@github-actions github-actions bot changed the title Add source generator that reads a JSON value as a const string at compile time. Add source generator that reads a JSON value as a const string at compile time. (OSOE-819) Feb 21, 2024
@Piedone
Copy link
Member

Piedone commented Feb 22, 2024

Hmm, very interesting! This would be especially useful for updates done by Dependabot.

@AydinE AydinE self-assigned this Mar 5, 2024
@AydinE
Copy link
Contributor

AydinE commented Mar 7, 2024

I've been looking into this issue and playing around with source generators as I've never worked on one before.
There are a few findings that I wanted to share here and get some feedback

The classes that will be marked with the attribute [ConstantFromJson("ChartJsVersion", "package.json", "$..['chart.js']")] will need to be marked as partial: also see Source Generators cookbook
I am assuming that it won't be an issue to mark them as partial but wanted to just call it out.

Also in order to make the package.json file accessible to the generator it would have to be marked as an "AdditionalFiles" in the .csproj as source generators do not have access to the filesystem directly.

I am setting this up as a separate project within this repo for now, let me know if there are any thoughts around that.

@Piedone
Copy link
Member

Piedone commented Mar 8, 2024

Partial shouldn't be an issue, nor AdditionalFiles I think but the latter also affects NuGet publishing. Perhaps it'd only result in an unnecessary file being bundled in the package, which is not a significant issue, but I don't know if it can break anything else.

@AydinE
Copy link
Contributor

AydinE commented Mar 8, 2024

Hey @Piedone,

The marking of the AdditionalFiles is actually for the projects that will use the generator not on the generator itself. So the consuming projects would have to mark those files in their own .csproj so that the generator can access them at compile time

@Piedone
Copy link
Member

Piedone commented Mar 8, 2024

Yeah, I understood it as such. Just that we publish all of our open-source projects using package.jsons on NuGet, so that'll affect their NuGet publishing (e.g. https://github.com/Lombiq/Orchard-Chart.js, https://github.com/Lombiq/Orchard-Content-Editors, https://github.com/Lombiq/Orchard-Data-Tables).

@Piedone
Copy link
Member

Piedone commented Mar 20, 2024

I get build errors locally with a fresh clone of OSOCE:

Build log.txt

image

Repeated builds also fail.

@Piedone Piedone reopened this Mar 20, 2024
@Piedone
Copy link
Member

Piedone commented Mar 20, 2024

The earlier OSOCE 509442daa9bdc7dc060d8a0196b8e803881af293 also shows this:

image

@sarahelsaig
Copy link
Member Author

Considering that it works with dotnet but not with VS, perhaps this is a VS bug. Then @AydinE please make a bug report.

@Piedone
Copy link
Member

Piedone commented May 3, 2024

Good job, @AydinE!

@Piedone
Copy link
Member

Piedone commented May 13, 2024

Reopening, since this breaks NuGet publishing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment