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

Conversation

zackargyle
Copy link

@zackargyle zackargyle commented Dec 9, 2017

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.

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.

Proposal

return (
<img
class="large-img"
src={imageSrc}

Choose a reason for hiding this comment

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

class -> className ?


## 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.


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.

@mattbasta
Copy link

Perhaps a solution to the memory leak issue is to require the developer to specify their own cache which they manage manually. For instance:

const myCache = new Map();

const RecycledCompoonent = (props) =>
  <MyComponent
    dangerouslyRecycleNode={{cache: myCache, key: props.id}}
    {...props}
  />;

This would also solve many unaddressed problems:

  • Third party libraries wouldn't need to worry about conflicting with IDs from other libraries
  • Developers could add their own cache eviction policy. This RFC is fundamentally a performance enhancement, and performance is not one-size-fits-all solution.
  • The cache could be traversed manually, flushed, or otherwise manipulated if a change in application state makes cached values invalid.
  • The cache key is not limited to strings.
  • Developers could implement their own .set() method that rejects nodes from the cache, potentially saving resources under memory pressure.
  • etc.

@zackargyle
Copy link
Author

zackargyle commented Feb 16, 2018

What if dangerouslyRecycleNode instead took in an actual node? The upside is that it'd be even simpler internally. The downside is that developers may create memory leaks since they control the caching mechanism.

class Page1 extends React.Component {
  onClick(event) {
    this.props.history.replace({ node: event.target });
    this.props.history.push('/new-route', { node: event.target });
  }
  render() {
    return (
      <div>
        <image
          src={...}
          onClick={this.handleClick}
          dangerouslyRecycleNode={this.props.location.state?.node}
        />
      </div>
  }
}

class Page2 extends React.Component {
  render() {
    return (
      <div>
        <image
          src={...}
          dangerouslyRecycleNode={this.props.location.state?.node}
        />
      </div>
  }
}

@vcarl vcarl mentioned this pull request Mar 12, 2018
@sophiebits
Copy link
Member

FWIW, one workaround you can do today is to render the node you want to move into its own React root (via a portal), and then manually move that container around using DOM operations. However it is clumsy and does require extra wrapper elements.

@gaearon
Copy link
Member

gaearon commented Feb 10, 2020

The transition use case is interesting and something @sebmarkbage has recently been thinking about. However, it is unlikely that recyling the DOM node is how we'll go about it. Instead, we might want to have a first-class way for components to specify their exit animations. This will likely tie into other Concurrent Mode features. So I'll close this particular RFC but we care about the use case.

Note that the "reverse portal" use case can be implemented in userland. I describe how here: facebook/react#12247 (comment).

@gaearon gaearon closed this Feb 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants