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

Store coordination (Conversation rules) #97

Merged
merged 2 commits into from
Nov 19, 2021

Conversation

diklimchuk
Copy link
Contributor

@diklimchuk diklimchuk commented Nov 3, 2021

What is it?

This merge request enables a new approach for defining communication between stores

Why is this needed?

The current implementation has some disadvantages:

  • Limits communication possibilities
    Only event to event and effect to effect mappings
  • Has strict contract
    Events are mapped without state and effects may access parent state
  • Defines strict parent - child relationship
    Instead of making stores as indepent as possible
  • Allows to access internal implementation details
    Events by nature contain a private store information. Internal events are named this way obviously to represent internal events
  • There's no way to trigger an event in the parent from the child store
    There are other possibilities to workaround this, but it seems like the right time to make this change.
    The case which lead to this change:
    • Parent store along with parent fragment are immutable and kept in the common module
    • Child stores are plugins to the parent store and fragment
    • Child stores need to trigger some event in parent

Why is the new approach better?

It has some advantages:

  • Allow to contruct any communication mechanism
    It has an api for defining communication contract
  • Communication contract may access only public api defined in the Store interface
    Instead of accessing implementation details

Is this api backwards-compatible?

No, the old api is kept to allow postponing migration

How to migrate to the new api?

There's no way to migrate easily to the new api, because there's no way to map events to events and effects to effects

How to migrate?

Effect to effect

Consume child effects in the parent and send effects directly OR consume effects in the fragment (Api for using multiple stores in the fragment is planned in some distant future)

Event to event

Replace consumable child events with effects

The main questions

  • Do you agree that this is an appropriate change?
  • Will you be able to handle migration?
  • Do you need more apis for easier migration?
  • Are there any structural incompetibilities which can't be implemented with new apis?

@diklimchuk diklimchuk force-pushed the store-contracts branch 2 times, most recently from a309a34 to e96c64d Compare November 3, 2021 07:49
@diklimchuk diklimchuk changed the title [WIP] Store contracts Store contracts Nov 3, 2021
@diklimchuk
Copy link
Contributor Author

@sergei-lapin, @opengamer29, this is a major api change. Please, take look at this carefuly. I want to add this functionality in some way, since we heavily use child stores in Haier.

Copy link
Contributor

@sergei-lapin sergei-lapin left a comment

Choose a reason for hiding this comment

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

Great feature!

*/
class BindingContract<ParentEvent, ParentEffect, ParentState, ChildEvent, ChildEffect, ChildState>(
val parent: Store<ParentEvent, ParentEffect, ParentState>,
val child: Store<ChildEvent, ChildEffect, ChildState>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Since one of the issues addressed is an attempt to make "parent" and "child" more independent then maybe it's better to move away from this concepts in naming as well?
Smth like parent -> first, child -> second (FirstEvent, SecondEvent etc.) seems a bit more appropriate here and in other files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't mind, let's see what @opengamer29 has to say

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes First/Second make more sense for me.

return this
}

override fun stop() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just an idea. Can it be helpful to save refs to contracts, that store involved to, inside stores to revoke contracts easily when a store is stopped?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Contracts already have no effect when either of the stores stops and you suggest store code complication. No.

@opengamer29
Copy link
Contributor

opengamer29 commented Nov 13, 2021

I'm pretty sure that @sergei-lapin told me about possible improvements in parent\child communications 2-3 weeks ago. And tadam... Did you discuss it before?

Also, it will be great to have it inside samples.

@diklimchuk
Copy link
Contributor Author

diklimchuk commented Nov 14, 2021

I'm pretty sure that @sergei-lapin told me about possible improvements in parent\child communications 2-3 weeks ago. And tadam... Did you discuss it before?

Also, it will be great to have it inside samples.

Did you discuss it before

No, if I remember correctly.

Also, it will be great to have it inside samples.

Javadoc and test cases already have api examples.

@diklimchuk
Copy link
Contributor Author

@sergei-lapin and @opengamer29

This is not something i would normally write but past months while writing a solution for this mr seemed too mysterious to me. This library is truly magical and I had a chance to withess this myself. I also had a bunch of strange coincidences. This is not a place to discuss such things, but one thing I will say. Last week's friday I oficially became an android teamlead at Haier. Yay.

I wonder, whether the only option for me was to become a teamlead since this was the only way to solve the inherent error introduced into the library at the date of creation by my own inability to see that it is brutally limiting or one-sided. I hope this explains how it turned out like that. I don't mention stores in futher text but always imply them. The old child-parent communication mechanism was built on the idea of complete child self-sufficiency. Without any ability for children to request anything from parents and them willingly delivering all possible information internally provided by free access to parent's internal events with the only purpuse - providing children with required independence. One the other side, parent had no knowledge of child's work and wasn't allowed to affect their behavior in any way, neither by sending events nor by sending state updates.

I've added a solution for the problems both of you desribed. Take a look and tell me if something could be improved. Another update. Now there are a lot more of api illustrations. I've expanded test cases with stories. Some of them are joyful. Some of them are nightmares. I'll add them with a separate mr to work a little bit longer on the details. Don't look if you don't want to know. Now there's the only one - look at the javadoc.

I've refined the original solution. Store contracts are replaced with store coordination. I like this new naming a lot more, because of the ability to unite more cases. Coordination not only implies equality, even means so. A better name for this is "Conversation Rules".

I hope new coordination mechanics will provide enough space for two-sided communications. Either way, the good old child-parent mechanic is still present. You both know where to look. If in any case you need me or there are some problems with the library functions, telegram and issues are always open For you. Choose your path. Wisely.

I will keep fixing bugs and hope add a lot more new features.
At least while one of you doesn't know who Elmslie is, the doors of this magic library will remain open.
Sometimes the only real way out is to walk all the way in.
See you on the other side. Salut. ✋

@diklimchuk diklimchuk force-pushed the store-contracts branch 2 times, most recently from 264d4ea to 0dd5c69 Compare November 18, 2021 02:21
@diklimchuk diklimchuk changed the title Store contracts Store coordination (Conversation rules) Nov 18, 2021
@diklimchuk diklimchuk merged commit 59f827a into vivid-money:main Nov 19, 2021
@diklimchuk diklimchuk deleted the store-contracts branch November 19, 2021 05:28
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.

3 participants