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

Add module-info.java #104

Merged
merged 12 commits into from
Apr 3, 2022
Merged

Add module-info.java #104

merged 12 commits into from
Apr 3, 2022

Conversation

overheadhunter
Copy link
Contributor

@overheadhunter overheadhunter commented Jun 18, 2021

This adds a module-info.java and fixes #103.

Note there are two unexpected but necessary changes:

  1. I had to disable builds using JDK 8. This is required for building. It does not affect the Java compatibility level, which stays at 7.
  2. Due to stricter access control mechanisms in modular apps, I had to fix resource loading, which used to facilitate classloader. However, this is not recommended and breaks easily in different scenarios. See this SO post for an explanation. Please take care to review this particular change thoroughly! Edit: Reverted due to concerns mentioned below

Futhermore, it is worth pointing out that the line exports com.nulabinc.zxcvbn.matchers; inside of the module-info is only necessary due to the fact that com.nulabinc.zxcvbn.matchers.Match is exposed public API. If this class can be moved to the top-level package, this line can be removed. So from a library user's perspective, both com.nulabinc.zxcvbn.matchers.* and com.nulabinc.zxcvbn.guesses.* will be hidden.

@overheadhunter overheadhunter marked this pull request as ready for review June 18, 2021 11:52
@vvatanabe
Copy link
Member

@overheadhunter

ResourceLoader.java solves the following problems.
#79

Without the ResourceLoader class, the above problem is likely to recur in various applications using zxcvbn4j.

@overheadhunter
Copy link
Contributor Author

overheadhunter commented Apr 3, 2022

The linked issue was caused by the fact that you used to use Thread.currentThread().getContextClassLoader() instead of getClass().getClassLoader().

However, even this is not "correct", see linked StackOverflow Post. I would encourage you to just retest the new implementation, as it is the way resource loading is intended.

Edit: I believe you could even do a simple getClass().getResourceAsStream("keyboards/qwerty.txt") (note the package-relative path).

@overheadhunter
Copy link
Contributor Author

To simplify review and not delay things further, I've re-added the ResourceLoader (added the described way of loading a resource as a first step, though). Note that the resource URL still needs to be absolute (tbh I wonder how it ever worked in the first place 🙈).

@vvatanabe vvatanabe merged commit 783fcc8 into nulab:master Apr 3, 2022
@vvatanabe
Copy link
Member

@overheadhunter Thank you! I will prepare to release 1.6.0

@overheadhunter overheadhunter deleted the patch-3 branch April 4, 2022 03:40
@vvatanabe
Copy link
Member

@overheadhunter I got an error in validation after publishing to sonatype. It will take some time to release it.

image

@vvatanabe
Copy link
Member

@overheadhunter Now I released zxcvbn 1.6.0. Please check Java 9 Modules.

@overheadhunter
Copy link
Contributor Author

@vvatanabe Works like a charm, thank you very much!

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.

Add module-info.java
2 participants