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

Async Unmounting #136

Closed
wants to merge 11 commits into from
Closed

Async Unmounting #136

wants to merge 11 commits into from

Conversation

brainkim
Copy link
Member

@brainkim brainkim commented Jul 21, 2020

WIP on Async Unmounting API.

Elements should be able to linger in the DOM for longer than they’re in the virtual element tree. This is primarily useful for exit animations. We don’t want every component to unmount asynchronously. After some dogfooding I think the following conditions feel the most ergonomic:

  1. The element should be a component element.
  2. The element should be a keyed element. This condition seem to catch a lot of false positives where we don’t actually want the rendered values to linger, and prevents unit tests from having to be changed. *
  3. The component is either an async generator component or the cleanup method is called with a callback which returns a promise. This is the main mechanism behind async unmounting. Bonus points if we can somehow exclude async generator components which return immediately from this logic. **
  4. The component is directly unmounted. Nested unmounts should not trigger lingering.

* Coming back to this list. I’m not 100% sure this should be a requirement.
** Coming back to this list again. I’m beginning to think we should exclusively use cleanup callbacks and ignore return() stuff. The reason being it’s pretty easy to deadlock an async generator and I don’t want lingering in that case.

Related issues in other repositories.

July 27th 2020
I have async unmounting mostly working. Check out the transitions directory in the examples, which does the d3 enter-update-exit letters thingy. I need to write tests, which will be a pain.

The only thing blocking merge is I wanted to do async unmounting while also making performance upgrades. Specifically, the introduction of a “remove” method on the internal renderer API, and coordination of the “arrange” and “remove” methods. I’ve made one pass, but my current thinking is we have to change the return types of the “update”/“commit” methods, which is a refactoring I just don’t feel like doing right now. This would involve bringing back to life the “dirty start”/“dirty end” optimizations. Probably will pick this back up in a month.

Open questions:

  • Should the return data include the start and end of the dirty range? If we do a flat boolean dirty flag, does that make arrange/remove optimizations worth it?
  • If we do dirty start, dirty end, should the start and end be inclusive-inclusive, or inclusive-exclusive?
  • Should the signature of arrange change?
  • Should we add hints to the remove method as well, and are there any other optimizations we can make to the arrange/remove call stuff. For instance, should we call arrange once with an empty array in the case of no shared children? Increase the coordination of arrange and remove?

@brainkim
Copy link
Member Author

Somehow moving src/index.ts to src/crank.ts wasn’t picked up by Git despite putting it in two separate commits. Might close and retry this PR rather than attempting to rebase 😓.

@brainkim
Copy link
Member Author

brainkim commented Mar 6, 2021

Probably gonna have to retry this effort from scratch because of changes. 😓

@brainkim brainkim closed this May 29, 2021
@Scott-HangarA
Copy link

Hi did this get picked up elsewhere or how can I follow this? Thanks!

@brainkim
Copy link
Member Author

brainkim commented Aug 17, 2021

@Scott-HangarA Weird coincidence! I’m literally diagramming how to implement this because I can’t go to sleep, and came back to this PR because I wanted to see my old notes. I’ll create a PR soon, but I should probably have done this as an issue to track it. It will probably be put into the next version (0.4), and I’ll reference any new work in this PR, so following this PR will still keep you up to date.

Here’s what I’m thinking about right now: How important is it that the position of exiting DOM nodes be preserved? Async unmounting would be so much simpler if we simply appended exiting DOM nodes to the end of whatever parent it was exiting from.

Pros of ignoring the “last position” of exiting nodes:

  • Easier to implement
  • You don’t have to worry about what happens when a parent element is rerendered while it has async unmounting children:
    • where should new nodes go relative to the exiting nodes?

Cons:

  • Can’t rely on layout in exit animations (this is actually a huge con)
  • Probably surprising behavior (another huge con)

@Scott-HangarA
Copy link

Thanks for the update! I could imagine a case where the exiting nodes might be needed in the same order. For example, using Material-ui it creates dropdowns/ drawers/ modals in nodes at the bottom of the page outside of the parent node. This raises the question (for me anyways) of what happens when someone has, let's say, a drawer open, and then navigates away? Below is an example from https://material-ui.com/components/drawers/
Screen Shot 2021-08-17 at 10 55 47 AM

@brainkim
Copy link
Member Author

brainkim commented Aug 17, 2021

@Scott-HangarA
Screen Shot 2021-08-17 at 6 26 02 AM

This was me trying to figure out where exiting nodes should go at 6 AM this morning haha. Like, if you have nodes A, B, C, D, and rerender D, A, where should B and C be placed as they’re exiting. For some reason, I thought the most ideal behavior was B, C, D, A and was sketching out, frankly, some insane algorithms to produce this ill-specified behavior, but I realized after taking a walk that the most ideal behavior would probably be D, B, C, A, as in, preserve the original index of exiting nodes in the new children, which is a much simpler algorithm to implement. It’s still non-trivial to implement, but at least I’m a bit clearer on what the exiting placement behavior should be.

The real mind-f*ck which I don’t want to deal with is when you have multiple sets of exiting nodes which overlap in terms of when they’re exiting, where should they all be placed relative to each other.

Yes, the trick these exiting animations use is to recreate the original layout of the exiting nodes using fixed positioning and css transforms, but ideally we’d want to be able to preserve the DOM nodes where they were originally so we don’t have to do all that layout measurement.

@brainkim brainkim deleted the async-unmounting branch October 5, 2021 21:07
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.

2 participants