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 plugin support for other templates languages #102

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

sarahelsaig
Copy link

@sarahelsaig sarahelsaig commented Dec 7, 2024

I've thought this through, and I think Roslyn C# script plugins make the most sense. (also to make it work with DLLs you'd have to publish OrchardCoreContrib.PoExtractor.Abstractions to NuGet as a separate library package - probably not worth the effort) I've included a unit test and sample plugin that hopefully gets the point across without being too niche. (The important change is in the Program.ProcessPluginsAsync method, the rest is just plumbing.)

Fixes #101

@hishamco
Copy link
Member

hishamco commented Dec 7, 2024

I just looked into the PR and noticed a dependency on Program.cs which is wrong for me. IMHO exposing the IProjectProcessor will make the tool extendable. I am still thinking if we need the -p or if we can make the tool open some possibilities for the others to add their own options

Let me hear your feedback on this

@sarahelsaig
Copy link
Author

I just looked into the PR and noticed a dependency on Program.cs which is wrong for me.

If you mean the ScriptOptions.Default.AddReferences(typeof(Program).Assembly) here, I've only used Program to expose all the libraries to the script the tool has already loaded. As I see it, this is not a security application so we can be very permissive.
For example this way the script has access to OrchardCoreContrib.PoExtractor.DotNet.CS too, not just OrchardCoreContrib.PoExtractor.Abstractions. If someone wants to have a different take on one of the existing processors, this could be a way to have your own custom (more opinionated) solution without it affecting the main repo, while re-using the auxiliary classes that are already in those projects. I don't see the harm in it, so could you explain why it's wrong for you?

IMHO exposing the IProjectProcessor will make the tool extendable.

I think it's not possible to expose just one type, you have to expose its whole assembly. We can change the above linked code to only expose OrchardCoreContrib.PoExtractor.Abstractions instead of everything, but that feels unnecessarily restrictive.

I am still thinking if we need the -p or if we can make the tool open some possibilities for the others to add their own options.

Sorry, I don't really understand. This -p approach is the simplest way to add broad extensibility that I could think of, while being less maintenance in your code base. If you want something more elaborate without the need to use a command line switch we can discuss, but I'm not sure if it's really necessary.

@sarahelsaig
Copy link
Author

Also by importing Program's assembly the plugin developer will have access to all the of external dependencies PoExtractor has already loaded. For example if I only included the OrchardCoreContrib.PoExtractor.Abstractions, then the example plugin will fail with the following error:

error CS0234: The type or namespace name 'Json' does not exist in the namespace 'System.Text' (are you missing an assembly reference?)

So now I'd have to include #r "nuget: System.Text.Json, 9.0.0" at the start of the script, and NuGet may have to download this version if it gets out of sync from what the tool itself uses. This is annoying imo.

@sarahelsaig
Copy link
Author

@hishamco Have you seen my replies? This is not super urgent yet but it would be good if we could have a solution before year's end, because we might need it next month.

@hishamco
Copy link
Member

I didn't look into the last comment, but the PR is opened in one tab, so I need to revise it today or tomorrow to proceed

@sarahelsaig
Copy link
Author

Hi @hishamco , any news with this?

NuGet.config Outdated
<clear />
<add key="NuGet" value="https://api.nuget.org/v3/index.json" />
<add key="OrchardCore" value="https://nuget.cloudsmith.io/orchardcore/preview/v3/index.json" />
<add key="nuget.org" value="https://api.nuget.org/v3/index.json" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The feed is already there?!!

Copy link
Author

@sarahelsaig sarahelsaig Dec 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooops, I didn't notice. I've reverted the name.

My point was just to remove the OrchardCore source. It's not even used and because of the central package management this kept giving warning NU1507: There are 2 package sources defined in your configuration.

If you need the Orchard Core preview feed in the future, I suggest adding the full <packageSourceMapping> configuration, as you can see in OrchardCore.Commerce.

foreach (var plugin in plugins)
{
var code = await File.ReadAllTextAsync(plugin);
await CSharpScript.EvaluateAsync(code, options, new PluginContext(projectProcessors, projectFiles));
Copy link
Member

@hishamco hishamco Dec 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why this is in Abstractions while it relies heavily on CSharpScript

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where should it be then?

@hishamco
Copy link
Member

@sarahelsaig Sorry for the late, I revised the code one more time still worried about the impact of dynamic linking, so I'm thinking for two simple approaches:

  1. Creating OrchardCoreContrib.PoExtractor.VueJs directly in the repo
  2. Reference Lombiq.VueJs to OrchardCoreContrib.PoExtractor

While this repo is Orchard Core Contrib it SHOULD help any OC project, I might think the first option is better, but I need to hear your feedback. Also, I will think for the last time about dynamic linking and its drawbacks

@sarahelsaig
Copy link
Author

Can you explain what is your worry about dynamic linking? What are the drawabacks? I don't see any impact unless you manually opt-in by using the command line switch. Otherwise I think it's safe with only very minimal overhead. I see it as a transparent improvement.

this repo is Orchard Core Contrib it SHOULD help any OC project

I fully agree with this sentiment. Note that since my original PR, I have seen other highly project-specific opportunities where this feature could be useful. In both of the cases below I could use a PoExtractor plugin to centralize other localization tech so OC can manage the actual localization and then serve it back to the associated tech. This will reduce the number of tools for the staff to learn to do the translations, and make complete translation management easier and safer.

  • We have a client project that uses OC as a headless server with a complementing node.js frontend. We could harvest the english l18n JSON files used by the frontend (similarly to the example plugin in this PR) and then serve back the other languages.
  • In the same project we also have to use a XSLT to generate some visualizations (it's a legal requirement). For now we manually maintain XML translation files for the languages we need in the MVP, but having a third localization tech in the project just for one small feature is very annoying.

I don't think we should create OrchardCoreContrib.PoExtractor.VueJs, not other OrchardCoreContrib.PoExtractor projects for the use-cases I mentioned above. This would quickly bloat your repo.

Reference Lombiq.VueJs to OrchardCoreContrib.PoExtractor

I'm not sure if I understand. If you mean using OrchardCoreContrib.PoExtractor as a library inside Lombiq.VueJs, your existing NuGet tool package doesn't make it possible. Even if you released a NuGet library package it would be pointlessly cumbersome to use in my opinion.

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 this pull request may close these issues.

Add plugin support for other templates languages
2 participants