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

Api: Forbid binding to subcomponets directly, tweak component factories. #154

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Jeffset
Copy link
Collaborator

@Jeffset Jeffset commented May 16, 2024

Before the patch, if a child component didn't have a builder its instance could be injected directly. Such behavior is confusing and vanilla dagger doesn't support that.

The reasons of why this behavior was here in the
first place are historical and date back to the time when yatagan didn't support child components factory methods, and relied solely on Module.subcomponents.

This patch removes the confusing behavior and makes it as in vanilla dagger.

Also, now every method in a component interface returning a non-root component is considered a component-factory. Before the patch only the methods with arguments were considered a factory. However, this imposes new restrictions
on how component instances can be exposed from component interfaces, as such usages will now be considered as factories, not entry-points. This leads to some tweaks in tests. But such cases seem of little use in the real world scenarios. And, anyway, that's still how vanilla dagger works.

This is a breaking change, but it's okay for 2.0.

Before the patch, if a child component didn't have
a builder its instance could be injected directly.
Such behavior is confusing and vanilla dagger doesn't
support that.

The reasons of why this behavior was here in the
first place are historical and date back to the time
when yatagan didn't support child components factory methods,
and relied solely on `Module.subcomponents`.

This patch removes the confusing behavior and makes it
as in vanilla dagger.

Also, now every method in a component interface returning
a non-root component is considered a component-factory.
Before the patch only the methods with arguments were considered
a factory. However, this imposes new restrictions
on how component instances can be exposed from component
interfaces, as such usages will now be considered as factories,
not entry-points. This leads to some tweaks in tests.
But such cases seem of little use in the real world scenarios.
And, anyway, that's still how vanilla dagger works.

This is a breaking change, but it's okay for 2.0.
@Jeffset Jeffset added the api Related to introducing new API or changing existing one label May 16, 2024
@Jeffset Jeffset added this to the 2.0.0 milestone May 16, 2024
Copy link

codecov bot commented May 16, 2024

Codecov Report

Attention: Patch coverage is 85.71429% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 83.37%. Comparing base (e8d3224) to head (d6bf9d2).

Files Patch % Lines
.../yatagan/codegen/impl/ComponentFactoryGenerator.kt 75.00% 0 Missing and 1 partial ⚠️
.../yandex/yatagan/codegen/impl/ComponentGenerator.kt 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main     #154      +/-   ##
============================================
- Coverage     83.71%   83.37%   -0.35%     
+ Complexity     1555     1538      -17     
============================================
  Files           191      190       -1     
  Lines          7548     7523      -25     
  Branches       1416     1412       -4     
============================================
- Hits           6319     6272      -47     
- Misses          727      746      +19     
- Partials        502      505       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Jeffset Jeffset requested a review from byar May 16, 2024 20:06
@Jeffset Jeffset marked this pull request as ready for review May 16, 2024 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Related to introducing new API or changing existing one
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant