[MSHADE-353] adding a generic relocation friendly transformer

[rmannibucau]

The generic transformer will actually delegate to other transformers for the actual processing to avoid to reimplement again and again the same logic.

//cc @rfscholte can you review it since you worked on the manifest flavor?

[tandraschko]

tested and works fine: primefaces/primefaces@9e8e2c3

[rfscholte]

Maybe I'm missing an important detail, but in this example is no relocation. So why do it like this?
And I would have expected that defining both transformers after each other would work, but that requires defining the resource twice. That's probably the thing you're trying to solve.

[rmannibucau]

This example is about composing transformers which is not something obvious for most people.
Defining each one after the other does not work because the relocation transformer is a delagate pattern which only works if there is another transformer saying it what it should handle. Goal is not to not define resource twice but to be able to decorate any transformer with this feature, including the ones with dynamic resource handling (pattern in canTransform for ex).

[rfscholte]

Nested transformers looks more like a XML representation of the Java solution, and it might also expose a design flaw in the plugin.
It probably makes more sense to start with the resources, and define its transformers:

<resources>
  <resource>
     <includes/>
     <excludes/>
     <transformers/>
  </resource>
</resources>

I understand that this is quite a different approach, but this looks way more clear to what's happening for the end user. Maybe the code can stay almost the same, but I would map this configuration to the actual transformer calls.

[rmannibucau]

Sorry @rfscholte , I don't get this proposal at all. You assume the resource configuration is an input of a transformer but a transformer can actually compute its resources and even create them on the fly without taking any input from the project sources so I don't see how it can work :s.

[rfscholte]

Based on your test the input is foo/bar.txt, right?

[rmannibucau]

Hmm, yes and no. It is foo/bar.txt cause it is an AppendingTransformer but the RelocatingTransformer is not aware of that detail. Think to a GraalVM transformer, it will create resources depending what is in the build so the files will not be configured statically. The RelocatingTransformer still works.

[elharo]

Are we going to proceed with this or should we close it?

[rmannibucau]

+1 to move forward, this feature will become important with jakartaee 9

[tandraschko]

+1
It works fine

[tandraschko]

any news?

[rmannibucau]

@rfscholte do you still think we shouldn't have this kind of transformer wrapper or are we ok to merge?

[melloware]

+1 this is needed

[rfscholte]

This proposal is too specific. What you're asking for is the ability to chain any transformers, it is not just about relocation. By limiting this we will have issues future related feature requests.
So the design should be changed to support that. You will probably need a new type of Transformer that can chain ResourceTransformers.

[rmannibucau]

@rfscholte hmm, what about the comment that a chain does not work compared to a wrapper?

[rfscholte]

Based on the example that is the problem you are trying to solve right now.

[rmannibucau]

@rfscholte a chain does not enable to inherit from dynamic resources so I don't see how you aim at making it work, did I miss sthg?

[rfscholte]

AFAICS your current integration test doesn't cover handle dynamic content, so I'm not not convinced that it should be part of this ticket. How would you handle a dynamic resource right now (apart from chaining)?

[rmannibucau]

@rfscholte it is by design since the canTransformResource impl delegates to underlying transformers so if the underlying transformer is dynamic then it works. Chaining does not work cause there is no context between transformers (and I think it would be a worse solution to introduce one since delegation is a sane pattern without any new API there). Hope it makes sense.

[rfscholte]

I still have issues with the argument of dynamic resources. The plugin should first collect all resources, both static and dynamic, followed by the transformations.

[rmannibucau]

@rfscholte the plugin yes but it means it requires another configuration or feature external to transformers. It can work but it is way less composable and becomes more specific. Concretely it means we will create a new chain of transformers propcessing transformed resources. In terms of architecture it sounds worse since then you will need post-post-transformers etc. That said, if you really think this kind of transformer does not belong to maven-shade-plugin, I can release it externally, not a big deal, just thought it was generic enough to be core.

[rfscholte]

Sounds like a good proposal because it hacks into the current flow in the plugin: drop this PR and maintain the transformer yourself.

[rmannibucau]

Ok, will release it on github to unblock people waiting after it. That said I'd still want to make it part of shade plugin cause it is just aligned on transformer model, not sure why you are saying it hacks it. Maven config is designed to support that case and chains commonly use delegation to enrich elements without having to fork part of the configuration or add another useless concept. For me we are exactly there. Not sure what I don't get.

Edit: pushed code there https://github.com/yupiik/maven-shade-transformers, we are creating a sonatype account to be able to push on central and will release it as soon as we get it

[melloware]

@rfscholte To add my two cents with the upcoming change from javax package to jakarta package you are going to see a FLOOD of projects needing this behavior from the Shade plugin. So it seems shortsighted to dismiss it as not fitting the Shade plugin paradigm.

Can you reconsider or offer a better alternative coding solution to the Shade plugin while still meeting the need?

[rfscholte]

@melloware you've seen my comments. I hope somebody else can try to come with a different approach, I simply need all my spare time to move Maven core forward, otherwise I would have tried it myself. I'm just recognizing a bad structure that I don't want to support like that.

[melloware]

Understood.