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

Add dangerouslyRecycleNode RFC #7

Closed
wants to merge 2 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 76 additions & 0 deletions 0000-dangerouslyRecycleNode.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
- Start Date: 2017-12-09
- RFC PR: (leave this empty)
- React Issue: (leave this empty)

# Summary

Allow a DOM node from an unmounted component to be reused within a newly mounted component. This is particularly helpful when doing cross-route transition animations, or forwarding video playback to a detailed view from a feed.

This is a new feature request, and would have no backwards incompatibility issues.

Choose a reason for hiding this comment

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

This would, however, be backwards incompatible with any existing component that--though unlikely--uses a prop named dangerouslyRecycleNode.

Copy link
Author

Choose a reason for hiding this comment

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

This could be fixed with a codemod to rename their prop.


# Basic example

The proposed solution is to add a `dangerouslyRecycleNode={id}` react internal prop that can be added to raw dom nodes (like `dangerouslySetInnerHTML`). If an unmounting node contains the prop, React caches the node and if a mounting node has the same `id`, it will reuse the exact DOM node from the previously unmounted component.

```js
class FeedPage extends React.Component {
changeRoute(id) {
this.history.push(`/feedItem/${id}/`);
}
render() {
const { feedItems } = props;
return (
<div>
{feedItems.map(({ imageSrc, id }) =>
<img
className="small-img"
key={id}
src={imageSrc}
onClick={this.changeRoute.bind(this, id)}
dangerouslyRecycleNode={id}
/>)}
</div>
);
}
}
class DetailPage extends React.Component {
render() {
const { imageSrc, id } = this.props.feedItem;
return (
<img
className="large-img"
src={imageSrc}
dangerouslyRecycleNode={id}
/>
);
}
}
```

# Motivation

Reusing DOM nodes between routes can be a wonderful user experience, but is impossible within React core currently. The library [react-overdrive](https://github.com/berzniz/react-overdrive) was the inspiration for the API of this RFC. This would be a fairly simple addition to React core that would allow for some beautifully declarative route transitions.

## Detailed Design

The proposal is to add a `dangerouslyRecycleNode` prop to raw DOM nodes. Internally on unmount if a node has the prop set, it will add it to the internal `domNodeCache[key]`. If another node with the matching prop + recycle id is then mounted, React will not create a new node, but will extract the cached DOM node and use it in the newly mounted component. It will then immediately remove it from cache.
Copy link

@kyleshevlin kyleshevlin Dec 11, 2017

Choose a reason for hiding this comment

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

Asking for some clarity here. Your use case is to maintain a DOM node for component/route transitions, but then you say to "immediately remove it from cache". What happens when the user wishes to use the Back button? Would we need to maintain the node in cache, or would we be able to setup the same transition, just in reverse?

Copy link
Author

Choose a reason for hiding this comment

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

That's a great point. Maybe it should remain in cache until there is no element with the corresponding ID. So don't remove it from cache immediately after mounting the new tree, but set a timeout after unmounting the recycleable node. The cache could have some kind of data structure like this?

type RecycleCache = {
  [id: string]: {
    node: HTMLElement,
    unmountTime: null | number,
  }
};

Choose a reason for hiding this comment

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

I think that would be an option. I think the other is to clear the cache but enable transitions in both directions somehow. I mean, in theory, the same DOM node could transition across more than one dismount of parent components. Having the ability to do this no matter how many destinations I think could be useful.


We will log an error to console (in dev mode) after 30s of a node being added to the cache if it is not reused.

## Drawbacks

The main drawback is around the lifecycle of the cached node from the unmounting component. This could potentially cause memory leaks.

The proposed prop API is named `dangerouslyRecycleNode` because one option is to keep it cached until a node with the matching `id` is rendered (which may never happen). Another option would be to keep the reference in cache for a certain amount of time, and since this will mostly be used for reuse between routes, that should not be too long. Unless you're codesplitting by route and have not preloaded the route you're going to next. That is why a cache expiration timeout is tricky. My preference is to keep it around until it is reused, and to keep the name as `dangerous` so people are aware that they can be causing memory leaks if they don't actually reuse the node anywhere.

## Alternatives
Unsure.

## Adoption strategy
This would be a new internal prop on raw DOM nodes. People can adopt however they want with no chance of backwards incompatibilities.

## How we teach this
Docs

## Unresolved questions
Cache expiration for recyclable nodes.