The thread. :slightly_smiling_face:

# decompose

[Arkadii Ivanov]

The thread. 🙂

[electrolobzik]

From my point of view forcing component to implement ComponentContext by straining the type parameter is simpler and more obvious solution. The compiler plugin can be useful in cases when someone needs components without this check, because you can just not apply this plugin in this cases. But I don’t know what are this cases and even if they are important, providing a separate gradle dependency with relaxed (legacy) set of methods will be probably simpler and clearer than creating this plugin.

[electrolobzik]

My logic is the following: If component by it’s nature should implement ComponentContext then it should be forced by the API. 🙂

[Arkadii Ivanov]

> If component by it’s nature should implement ComponentContext
The current design is that it's optional but recommended. Not all developers love the idea of exposing ComponentContext from the component implementation classes, and they just use the context as parameter.

[Arkadii Ivanov]

Another idea of adding the check is to add a method to ComponentContext like fun onComponentUsed() {} (or similar). Then in DefaultComponentContext we can increment the counter and throw if it's >1. Then create a compiler plugin (perhaps KSP) that would generate the following code for every class that implements ComponentContext.

init { onComponentUsed() }

[Arkadii Ivanov]

Though, I can imagine someone creating normal classes (not components) that also delegate ComponentContext for convenience, just to extract some navigation logic. I might be event doing it myself, don't remember. So there are plenty of corner cases. 🙂

[electrolobzik]

This looks like 2 different strategies, both should work.

In the first we accept that there are 2 different ways to use the approach: with and without using the ComponentContext. And provide 2 different artifacts depending of user choice.

In the second we try to keep the single artifact and make it universal by applying optional checks (via compiler plugin for example).

Both this approaches will probably work, but both have some tradeoffs, just different. First gives more strait and simple API for both cases, but makes the library design a bit more complex by dividing artefacts. The second is probably a bit easier to maintain, but makes the solution more complex, involving code generation with all that downsides.

[Arkadii Ivanov]

Let me write down some facts.

1. Passing ComponentContext to a child component is bad because the child component is not aware of any keys that are also used by the parent component. E.g. the child component may stateKeeper.register(key = "saved_state"), which is also being done in the parent. This will crash if ComponentContext is reused.
2. It's ok to extract some logic from a component to a separate class.
a. It's ok to pass ComponentContext to those classes.
b. It's ok to delegate ComponentContext in those classes.
c. There will be a crash if there are two classes that use the same key for StateKeeper#register , or childStack , etc.
This leads me to a conclusion that components are no different from just extracted classes. We can also require using childContext when passing ComonentContext to extracted classes. But this is also error-prone and will lead to runtime errors if the developer forgets to use childContext and the check is triggered.

This leads me to the conclusion that it would be better to check if we can improve the error messages when keys are reused. Because this is the actual problem. If somehow we will manage to replace errors like "Another supplier is already registered with the key: Blah" with something like "Can't create another Child Stack with the key: Blah. Make sure you are not passing the parent ComponentContext to a child component. If your component has two Child Stacks, then specify a key for each of them.", then this would be much better. This is also a runtime check, not much different from a dedicated runtime check for passing parent ComponentContext to children.

[electrolobzik]

I not totally sure I follow the logic, but if I do, I don’t understand how changing the error messages will help to find where exactly the wrong context is passed. On the other hand if you require childContext in initialization of the component (or extracted class) it would be obvious where the error is.

[electrolobzik]

Also there are the cases, when no crashes will happen, by lifecycle events simply will not work correctly. This is also an important case which ideally should be covered.

[Arkadii Ivanov]

Also there are the cases, when no crashes will happen, by lifecycle events simply will not work correctly

I don't see how this can be covered. We can't distinguish between normal classes and component classes. It might be valid to pass the parent ComponentContext to a normal class. Currently - the way how we pass ComponentContext defines whether the class is a component or not (components receive child ComponentContext, whereas normal classes receive parent ComponentContext).

We could mark a component with a special interface - interface Component . But I'm not sure if it's a good idea to introduce the interface just for the sake of runtime checks.

I will need to think about it.

[electrolobzik]

Introducing the interface for components is exactly what I proposed. If semantically components have some restrictions it in my opinion makes total sense to add this restrictions in the type system. It is not just about runtime checks it is also about clarity.

[Arkadii Ivanov]

The thing is that I don't want to enforce any base interfaces or classes for components. This is because currently Decompose navigation can be used to manipulate trees of arbitrary classes. So this is a feature I don't want to break.

[electrolobzik]

yeah, it can be a separate set of types. so if you want to use rich components with lifecycle and instanceKeeper you use enforced interfaces and if you don’t - just use regular ones. It will separate the different use cases.

[Arkadii Ivanov]

Currently, we can create a child component by simply using childContext function. This means that the user may add the Component interface to the component class, or may not. There is no compile-time check, the interface is simply not required for the system to work. And it might be a mistake to pass the parent context instead, or it might not.

Also, we have the children navigation. Currently it doesn't enforce any base interfaces for child components - the type is simply T : Any. This is by design. So again, it's optional for the user whether to add the Component interface or not.

[electrolobzik]

Do you think if it can be important to use both cases in the same app? If not it could be just 2 separate dependencies: with and without type restrictions.

[Arkadii Ivanov]

I think we could do the following. Everything below would be an opt-in behaviour.

1. Add a method like checkComponentContextUsage() to ComponentContext interface.
2. Treat every class that implements ComponentContext interface as a component.
3. Create a compiler plugin (e.g. KSP) that adds init { checkComponentContextUsage() } to every component class.
By enabling the compiler plugin a user opt-ins to this behaviour and gets runtime checks. Now if they want to extract some component logic to a separate class AND they want to also implement ComponentContext in that class, they have to use childContext , which doesn't hurt actually.

But KSP for KMP is still experimental AFAIK. Plus I'm not sure I have capacity for this. 😐

[Arkadii Ivanov]

> If not it could be just 2 separate dependencies: with and without type restrictions.
This will introduce significant segregation and duplication on the API surface. Plus it's possible that a user needs both use-cases in one module.

[electrolobzik]

yes, it will. and If you think that it is important to keep possibility of using both use cases in one app it is not an option.

[electrolobzik]

The main reason why I proposed that solution with enforcing API was the fact, that for the newcomer, as I am it is not obvious that there is some logic underneath why passing the parent context is bad idea. If you didn’t explain this I would hardly understand it and I think most of users probably go through this bugs. It can be slightly improved by changing documentation, but will not solve the problem in the root. From my point of view the root of the issue is the fact that the API, being made on purpose universal (and this is really elegant in this part), doesn’t self-document the proper use of it. Adding some type restrictions can help the developer better understand the logic or probably at least become curious how it works.

[Arkadii Ivanov]

Thanks Roman for all your input and feedback! I think we should definitely grab some low hanging fruits and improve the docs, as a first step. Filed https://github.com/arkivanov/Decompose/issues/634.

The next step is to investigate some options like compiler plugin. Filed https://github.com/arkivanov/Decompose/issues/635.