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 support for module bundles #2432

Merged
merged 2 commits into from Sep 11, 2019
Merged

Add support for module bundles #2432

merged 2 commits into from Sep 11, 2019

Conversation

marcospassos
Copy link
Contributor

@marcospassos marcospassos commented Aug 24, 2019

This PR adds support for module bundles.

This feature is useful in cases where you have explicit dependencies on other modules, but you have to ensure that these dependencies will be registered as well, making it easier to depend on them without worrying about transitive dependencies.

I propose making it a built-in feature, rather than a composite module in the userland, due to the module deduplication feature that would be not able to detect the aggregated modules.

@marcospassos
Copy link
Contributor Author

marcospassos commented Aug 24, 2019

A good use case is Jdk8Module. Several modules depend on it, but currently, there is no way to guarantee that it will be installed for the module to work correctly.

With this new feature it becomes easy as:

public Iterable<? extends Module> getDependencies() {
    return singletonList(new Jdk8Module());
}

@cowtowncoder
Copy link
Member

This is kind of change that should ideally be discussed on dev mailing list first; or, if not, first filing an issue explaining approach. It is not an absolute requirement (it is possible to use PR id in release notes).

As to change itself, I am not sure what to think: I can see possible benefits at high level, but not yet practical ones. In case of Jdk8Module, for example, would this be for a hypothetical module that requires support of Optional (or similar)?

@marcospassos
Copy link
Contributor Author

marcospassos commented Aug 27, 2019

This is kind of change that should ideally be discussed on dev mailing list first; or, if not, first filing an issue explaining approach. It is not an absolute requirement (it is possible to use PR id in release notes).

Sorry, I opened the PR for discussion and for demonstrating a proof of concept.

In our case, we've several modules that depend on the Jdk8Module. We also have modules that depend on other modules, e.g.:

  • Module A depends on Jdk8Module
  • Module B depends on Module A and Jdk8Module
  • Module C depends on Module B and Jdk8Module

When you split the serialization modules into fine-grained modules, it's much easier to declare the dependencies at the module level rather than declaring the dependency every time you have to use it.

@cowtowncoder
Copy link
Member

Ok. I will have to think about this a bit more. I can see potential value here, just want to think through ramifications.

Would it make sense for you to ask about this on mailing list; just to see if others would find this immediately useful? And there might be some useful additional/related ideas. I am not asking for any vote of approval or such, just... some more sanity checking.

@marcospassos
Copy link
Contributor Author

marcospassos commented Aug 27, 2019

I've fixed the mentioned issues. I'll drop an email to gather more feedback.

@cowtowncoder
Copy link
Member

Cool, sounds good. I like this idea in that it's something I had not thought of, but seems to solve an existing issue. And concept of granular modules is pretty cool -- the only concern I have is that the solution both works and is extensible (if need be), and does not add new failure modes.

@marcospassos
Copy link
Contributor Author

Hope to see it as part of 2.10 :)

@marcospassos
Copy link
Contributor Author

@cowtowncoder let me know if you need any further help with this PR.

@cowtowncoder
Copy link
Member

Will do -- right now I am only limited by time, but should be getting this merged this week.

@cowtowncoder cowtowncoder merged commit 76fb342 into FasterXML:2.10 Sep 11, 2019
cowtowncoder added a commit that referenced this pull request Sep 11, 2019
@cowtowncoder
Copy link
Member

Merged, will be in 2.10.0. Thank you for contributing this!

cowtowncoder added a commit that referenced this pull request Sep 11, 2019
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

2 participants