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

Do not create permanentNavModel inside ParentNode. Provide it via constructor to ParentNode #606

Merged

Conversation

KovalevAndrey
Copy link
Collaborator

@KovalevAndrey KovalevAndrey commented Oct 2, 2023

Description

Fixes: #604

Using PermanentChild composable function, we're delaying adding a child to the ParentNode until PermanentChild is invoked. This can lead to many problems as the appropriate Node will be created only when composable is invoked. Additionally, there's a lot of complexity associated with making sure that we're using only the same instance of PermanentNavModel in the cases when one is provided to the constructor.

This PR removes internally created PermanentNavModel and forces users to provide it from outside

Check list

  • I have updated CHANGELOG.md if required.
  • I have updated documentation if required.

@KovalevAndrey KovalevAndrey added the bug Something isn't working label Oct 2, 2023
@KovalevAndrey KovalevAndrey modified the milestones: 1.3, 1.4 Oct 2, 2023
CHANGELOG.md Outdated Show resolved Hide resolved
.value
.find { it.key.navTarget == navTarget }
?.let { childOrCreate(it.key) }
?: throw IllegalStateException(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will this break the code within StatefulNode1.kt?

Just so I understand, it's now mandatory to pass in a NavTarget to the PermanentNavModel constructor?

Perhaps we should update PermanentNavModel to throw an IllegalStateException if there are no NavTarget's provided? Could we also consider removing the vararg then, as this will definitely lead to issues

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good catch

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should just ignore if there is no child of the specific type. Otherwise we break functionality where you add a child later. Or maybe provide some optional renderer function.

Just so I understand, it's now mandatory to pass in a NavTarget to the PermanentNavModel constructor?

Why it should be so? We still have add/addUnique operations to add nodes later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I feel like if you have a logic of adding a child later at some point it shouldn't be a part of PermanentNavModel

Copy link
Collaborator

Choose a reason for hiding this comment

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

perhaps leave it as is for now, and consider changing this behaviour in 2.0?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@CherryPerry my usecase is that I have a PermanentChild that is added immediately (via the constructor) and is only shown once the data is loaded (unfortunately in my usecase the ParentNode is responsible for when to show the PermanentChild based on Output of whether the state is loaded or not)

Without this change, the PermanentChild would not correctly start the business logic when added via the PermanentChild constructor (well actually it can, but it renders the wrong one at the moment)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Anyway, we are breaking existing use case without providing alternative, when you want to add your permanent child later after some flag is triggered. Now you can via if (myCheck) PermanentChild(NavKey.AddMe).

Copy link
Collaborator Author

@KovalevAndrey KovalevAndrey Oct 2, 2023

Choose a reason for hiding this comment

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

I reverted it back so we can add keys later

Copy link
Collaborator Author

@KovalevAndrey KovalevAndrey Oct 2, 2023

Choose a reason for hiding this comment

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

Or we can observe permanentNavModel in composition to render permanent children

val isAddMeExists = permanentNavModel
    .elements.
    .mapState(scope, SharingStarted.WhileSubscribed()) { navElements ->
         navElements
             .find { it.key.navTarget == navTarget } ?: false
     }
     .collectAsState()

if (isAddMeExists) { PermanentChild() } 

otherwise it's difficult to track issues where you forgot to add a key

Copy link
Collaborator Author

@KovalevAndrey KovalevAndrey Oct 2, 2023

Choose a reason for hiding this comment

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

Perhaps, can be added to an overloaded PermanentChild while default one fails

@KovalevAndrey KovalevAndrey merged commit e4f4076 into bumble-tech:1.x Oct 3, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants