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

Jigsaw modules support without dropping support for Java 8 #131

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Auties00
Copy link

@Auties00 Auties00 commented Feb 10, 2023

Hello,
I've added support for Java 9+ modules using the moditect plugin. This doesn't require dropping support for Java 8. I've done some testing and everything looks to be working. The second commit probably changed the line breaks in the pom so it looks like a lot of code has changed, but I practically just added the moditect plugin and changed nothing else.

@@ -1,10 +1,9 @@
module ezvcard {
open module ezvcard {
Copy link
Owner

Choose a reason for hiding this comment

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

Must the entire module be declared "open" to resolve the issue with the HTML writer?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, as far as I can tell they are using reflection to find and initialize something that has to do with the html - writer. You can't declare a single package as open, the whole module needs to be

Copy link
Owner

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in the latest commit.
opens ezvcard.io.html; looks to do the same thing as you said!

@@ -1,10 +1,9 @@
module ezvcard {
open module ezvcard {
Copy link
Owner

Choose a reason for hiding this comment

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

What happens if a user decides to exclude the JSON or HTML dependencies from their project because they do not need to use those vCard formats? (see: https://github.com/mangstadt/ez-vcard/wiki/Dependencies)

Copy link
Author

Choose a reason for hiding this comment

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

Oh that's a good question, those should be probably marked as static then. I'll test it asap

Copy link
Author

Choose a reason for hiding this comment

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

Fixed it with the latest commit.
I marked all of the dependencies you flagged in the pom as optional to be static.
(com.fasterxml.jackson.core is not static because it's not marked optional in the pom, only the databind is)

@mangstadt
Copy link
Owner

I assume that most people are not building ez-vcard themselves and are just downloading the JAR file from the central repository using a build system like Maven. Does this moditect plugin benefit those people in any way, or is it just useful for people who want to build the project themselves?

@Auties00
Copy link
Author

I assume that most people are not building ez-vcard themselves and are just downloading the JAR file from the central repository using a build system like Maven. Does this moditect plugin benefit those people in any way, or is it just useful for people who want to build the project themselves?

What this plugin does is compile a module-info to bundle into the main jar(you check that it's there using for example 7zip). When importing the library using Java ≤ 8, the module info will just get ignored as Java has no idea what do with it, but on newer versions it will detect the module info and treat the library as a named module. The alternative to this approach is to use the Automatic Module Name property in the manifest, but imo this is better as you have a real module info. I exported every package to preserve backwards compatibility, but you could theoretically only expose public APIs now for example, which is very useful imo.

@Auties00
Copy link
Author

Auties00 commented Feb 11, 2023

Btw I would recommend not exporting any of the io implementation(ezvcard.io and all of its sub-packages), ezvcard.util.org.apache.commons.codec, ezvcard.util.org.apache.commons.codec.binary and perhaps the util class. This could break backward compatibility if someone on Java >= 9 using the module system(aka they have a module-info in their project) is also using a class from those packages. The call is up to you, but imo it's always better to keep it as encapsulated as possible by exporting only the packages that provide APIs that should be publicly available

@Auties00
Copy link
Author

Any updates @mangstadt ?

@mangstadt
Copy link
Owner

Thank you for your work on this, but I don't think it adds enough utility to make the extra complexity worthwhile.

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.

2 participants