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

chore(deps): upgrades JAXB dependencies for feign-jaxb on Java17 #1700

Merged
merged 2 commits into from Jul 21, 2022
Merged

chore(deps): upgrades JAXB dependencies for feign-jaxb on Java17 #1700

merged 2 commits into from Jul 21, 2022

Conversation

smlgbl
Copy link
Contributor

@smlgbl smlgbl commented Jul 21, 2022

No description provided.

@smlgbl smlgbl changed the title feat: upgrades JAXB dependencies for feign-jaxb chore(deps): upgrades JAXB dependencies for feign-jaxb on Java17 Jul 21, 2022
Copy link
Member

@velo velo left a comment

Choose a reason for hiding this comment

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

Looking good

@velo velo merged commit 53c7ab6 into OpenFeign:master Jul 21, 2022
@sandalovsergey
Copy link

@smlgbl @velo Hi!

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!

@smlgbl
Copy link
Contributor Author

smlgbl commented Feb 8, 2023

The jaxb deps were removed in Java17, so they need to be pulled in explicitly.

@sandalovsergey
Copy link

@smlgbl all right, one more question

You have added jakarta.xml.bind-api if jdk >=17. See https://github.com/OpenFeign/feign/pull/1700/files#diff-849baa0d8ff067821d75f011846fa2b18607333f551aa5080d5fe28d62081539R85

This artifact contains jakarta.xml.bind-package only. My question is: why do we need this dependency?

If we look at the code, there is no jakarta.xml.bind-package usage. But there is javax.xml.bind-package usage. F.e. see
https://github.com/OpenFeign/feign/blob/master/jaxb/src/main/java/feign/jaxb/JAXBEncoder.java#L19
https://github.com/OpenFeign/feign/blob/master/soap/src/main/java/feign/soap/SOAPEncoder.java#L22

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?

@smlgbl
Copy link
Contributor Author

smlgbl commented Feb 8, 2023

I think you are right. It would be better to do it the way you suggest in #1918
You can go ahead and undo the change.

Kudos for making sure your artifact stays clean. Seriously.

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.

None yet

3 participants