maven-shade-plugin
maven-shade-plugin copied to clipboard
apache
[MSHADE-353] adding a generic relocation friendly transformer
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?
tested and works fine: https://github.com/primefaces/primefaces/commit/9e8e2c3b81005f5f8ef8cd77c5a8a42449c8a327
+1 to move forward, this feature will become important with jakartaee 9
@rfscholte do you still think we shouldn't have this kind of transformer wrapper or are we ok to merge?
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.
@rfscholte hmm, what about the comment that a chain does not work compared to a wrapper?
Based on the example that is the problem you are trying to solve right now.
@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?
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)?
@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.
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.
@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.
Sounds like a good proposal because it hacks into the current flow in the plugin: drop this PR and maintain the transformer yourself.
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
@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?
@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.