-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
chore(deps): upgrades JAXB dependencies for feign-jaxb on Java17 #1700
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good
Can you please explain what was this PR done for? Afaik feign-jaxb still using javax packages intead jakarta. See https://github.com/OpenFeign/feign/blob/master/jaxb/src/main/java/feign/jaxb/JAXBContextFactory.java#L21 So please clarify those changes for me. Thanks in advance! |
The jaxb deps were removed in Java17, so they need to be pulled in explicitly. |
@smlgbl all right, one more question You have added This artifact contains If we look at the code, there is no Did you add it in order to introduce a transitive dependency in clients code? AFAIK it is a bad practise to depend on transitive ones. I ask you in case of this issue: #1918. It seems that it needs to add a separate module(f.e. feign-jaxb-jakarta) for support jakarta in feign-jaxb. So before this I want to understand the reason of this PR. What it had to change? Why can't we do without this dependency? |
I think you are right. It would be better to do it the way you suggest in #1918 Kudos for making sure your artifact stays clean. Seriously. |
* feat: upgrade JAXB dependencies for feign-jaxb * feat: upgrade JAXB dependencies for feign-jaxb for Java17
* feat: upgrade JAXB dependencies for feign-jaxb * feat: upgrade JAXB dependencies for feign-jaxb for Java17
No description provided.