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

proposal to detach children before self #612

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

Conversation

davissuber
Copy link
Contributor

@davissuber davissuber commented Aug 23, 2023

This pull request stems from #589 and attempt to swap the detachment order between self and any attached children to avoid a small window in which the child is active and parent is not.

Note that this only updates the Android part - the corresponding iOS behavior remains "detach self before children" even after this pull request.

@tyvsmith
Copy link
Member

Has this been verified in the Uber monorepo? Tests passed and apps worked?

@davissuber
Copy link
Contributor Author

Has this been verified in the Uber monorepo? Tests passed and apps worked?

No, not yet - was creating this pull request to collect information on whether to do this at all (and if yes, gather information on what would be considered a sufficient test)

@tyvsmith
Copy link
Member

Suggest a first pass with a custom built dependency to understand the scope of the migration

@davissuber
Copy link
Contributor Author

davissuber commented Aug 24, 2023

Update: After smoke testing with Uber's big apps, decided to limit the change to only move the "detach self" ahead of "detach children" and leave the "will detach" position untouched

}
assertThat(selfInteractorDetachIndex).isGreaterThan(-1)
assertThat(childRouterDetachIndex).isGreaterThan(-1)
assertThat(selfInteractorDetachIndex > childRouterDetachIndex).isTrue()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we are detaching child before parent now, the router detach is no longer the final event.

@@ -206,11 +206,11 @@ protected constructor(
public open fun dispatchDetach() {
checkForMainThread()

interactorGeneric.dispatchDetach()
willDetach()
Copy link
Contributor

@psteiger psteiger Sep 22, 2023

Choose a reason for hiding this comment

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

Should the router willDetach come after interactor detach? IMO this should be the last line. What issues did you encounter with this (if any)?

I'm thinking of:

    for (child in children) {
      detachChild(child)
    }
    interactorGeneric.dispatchDetach()
    willDetach()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I had noticed was that some existing RIBs in Uber's app are overriding the willDetach() (and sometimes even call detachChild() themselves e.g. BikeHomeRouter) and/or do other sorts of clean up stuff that needs to happen before detachChild() is called by Router.

I think those seemingly ani-pattern-ish usage may be symptoms of the fact that we are detaching parent before child.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you are right.

I've been thinking about this, there's nothing super wrong with calling willDetach before everything else. It can be understood as a signal that router X is starting its detach process, which now will begin with detaching children (instead with detaching self). That makes sense.

Copy link
Contributor

@psteiger psteiger left a comment

Choose a reason for hiding this comment

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

If smoke test went fine, I think this is a welcome change. We should consider pushing for iOS changing it as well, for symmetry .

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