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

Reparenting API #34

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

Reparenting API #34

wants to merge 22 commits into from

Conversation

dantman
Copy link

@dantman dantman commented Mar 12, 2018

View formatted RFC

Terminology and naming for the createReparent function are up for discussion.

@streamich
Copy link

streamich commented Mar 12, 2018

Does this RFC suggest that React would move around the actual DOM nodes? Is it technically possible? For example, if one of the child node was a playing <video> element, by removing it from DOM and reinserting into a different parent, would it still be playing?

@dantman
Copy link
Author

dantman commented Mar 12, 2018

@streamich Yes, DOM nodes would be detached with removeChild if the reparent tree is not rendered, and re-inserted later with appendChild/insertChild when added back.

If one of the children of the reparent is a video element this does imply that the video element inserted in the other parent would be the exact same video element.

I cannot make guarantees however about the video still playing. Even if you keep the same instance of the dom node, some DOM elements have quirks when you move them around the dom tree. I'm not proposing any special hacks to handle that. So the video will be the same, but if videos stop playing when you appendChild them into another parent then videos will stop playing when you reparent them.

Though on the topic of playing videos. Take a look at my other "registered prop as ref" RFC, one of my examples is a custom Playing implementation. Which would make the video element a controlled component in regards to playing (like how inputs are controlled when you pass a value to them). Theoretically if you reparented a video with a controlled Playing attribute then even if the browser stopped it the controlled nature would make it resume after it is reparented (though you might have to ignore a pause event if the browser fires one).

@streamich
Copy link

streamich commented Mar 12, 2018

...if you reparented a video with a controlled Playing attribute then even if the browser stopped it the controlled nature would make it resume after it is reparented...

I think it would reset the currentTime of the video, too.

@dantman
Copy link
Author

dantman commented Mar 12, 2018

I think it would reset the currentTime of the video, too.

Well. If you really wanted. You could build a <Video> component which would make all the state in a <video> tag controlled (playing, currentTime, etc...) in the normal way for react (register events to keep state up to date like firing setState every time the video time change, map things to props and change them when props change, etc...). Then this RFC would guarantee that if your <Video> was reparented the React component would stay intact with all its state and in componentDidUpdate you could ensure that any controlled state that the browser reset from the reparent would be restored by your Video component.

@reverofevil
Copy link

reverofevil commented Mar 12, 2018

@dantman The whole point of reparenting is to keep intact all and any of the states of element that are not controlled by React.

Imagine yourself making something like Jupyter. You have a component that wraps CodeMirror, and you'd like to drag and drop rows of the document so that all the state of CM stays the same: selection, focus, scroll etc. There is no way to control all of this state by the component because CM doesn't provide API methods to control every single part of it. In the unlikely case that controlling all props works out with a <video> element, that's fine, but you need no reparenting for that. Reparenting exists exactly for the cases when you cannot do it.

@reverofevil
Copy link

@streamich Yes, it's as simple as appendChild on a component that is already appended somewhere else, without any removeChild calls.

@streamich
Copy link

streamich commented Mar 12, 2018

I guess, for React just re-parenting and leaving it up to the user to deal with consequence is fine as long as it is opt-in. Here is a short list of consequences though:

  1. <video> will stop and lose its state
  2. <audio> will stop and lose its state
  3. <iframe> will reload
  4. Any focused elements will lose their focus?
  5. Very important one: any .swf Flash players will reload 🤣
  6. Probably a bunch of other things will lose their state or reload.

@vcarl
Copy link

vcarl commented Mar 12, 2018

Seems to have some overlap with #7? dangerouslyRecycleNodesounds like a similar concept to me, though I admit I haven't deeply read through either proposal.


A Reparent is tied to a component, when this component is unmounted the reparent is unmounted and the detached tree is discarded. This allows reparents to hold detached trees without doing so in a permanent way that would leak memory.

A Reparent has an additional `.unmount()` method, this can be used to force the reparent to unmount and discard its tree like it would when the component it is tied to unmounts. This allows reparents to be hoisted upwards and used for dynamic content and explicitly unmounted when necessary.
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 this conflicts w/ the previous paragraph. If a reparent can be hoisted upwards, and the component it was tied to is unmounted, then it gets unmounted and discarded. Is there a method for changing the owner of a reparent?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry for not being clear. By hoisting for dynamic content, I mean you can move the React.createReparent(this) higher in the tree (hoisting), this higher node you use this from is the "component it is tied to"/owner, you can pass the reparent down to children using context/props and use it in descendants, then if the dynamic reparent is no longer needed you can call .unmount() (usually at the higher node) in order to unmount and discard whatever you put inside the reparent tree.

Perhaps I should define a glossary of things like 'state tree', 'dom/native tree', 'reparent owner', etc... in the RFC and use those terms to make things clear.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, so not at runtime, but during development. Got it.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, sorry. I thought it was clear by using "hoisting" because I normally when I hear "hoisting state" it's referring to moving the state = {} and setState definition from one Component to a parent component.

@dantman
Copy link
Author

dantman commented Mar 12, 2018

@polkovnikov-ph
There are dom states that do not get discarded when you call removeChild on them. And the ability to retain React Components while reparenting is just as important.

But my intent with mentioning removeChild is for the situations where you intentionally detach a tree and hold on to a reference to later mount it.

class MyComponent extends Component {
  content = React.createReparent(this);

  render() {
    const {show, children} = this.props;
    const content = this.content(<Fragment>{children}</Fragment>);

    if ( show ) {
      // Show the contents
      return content;
    } else {
      // Don't render the contents, the dom/component tree will not be
      // present in the page but it will be retained as a detached tree
      // and later re-used when `show` becomes true again.
      return null;
    }
  }
}

If you move a dom node from one parent to another in a render such that the reparent is never left out of the react component tree, I do think it would be ideal for React to just use appendChild/insertChild to move the node instead of using removeChild and then inserting it in the new parent.

If there are dom states that get discarded when you removeChild->appendChild in the same tick but not when you just appendChild. I can add a note to the design/implementation section noting that the implementation should not use removeChild unless the tree is actually being detached and should only use appendChild/insertChild calls when it is only necessary to move dom nodes.

I can also add a limitations section noting the dom nodes that lose state when you reparent them with appendChild.

Would that be sufficient?

@reverofevil
Copy link

I'm unsure, but it seems that it's possible to create a loop of reparenting components that locally looks as if it's possible to use only appendChild on them, but it actually requires at least one removeChild to break the loop. I'm too tired today to convert this idea of counterexample into code tho :/

@dantman
Copy link
Author

dantman commented Mar 12, 2018

To dive more into this issue of removeChild/appendChild and state loss I've built a series of simple dom reparent tests that will reparent an element using just appendChild, removeChild + appendChild, and using a delay before re-insertion. Feel free to propose other elements

https://dantman.github.io/dom-reparent-tests/

Summary

  • Chrome is the only browser that pauses video/audio when you reparent something with appendChild
  • Firefox and Safari won't even pause the video when you removeChild+appendChild, until you put a delay between the calls, they'll just keep playing the video at the new position
  • IE11 was the only browser that had any difference between appendChild and removeChild+appendChild – pausing the video when you removeChild+appendChild. Though given that Chrome always pauses, this is actually better than Chrome!
  • Absolutely none of the browsers I tested lost the current position of the video, even when the video was detached from the tree and a delay was used before putting it back. The suggestion that currentTime gets reset seems to be invalid.
  • iframes do reload in all browsers, no matter what. This is just a limitation that will have to be accepted for iframes as even just using appendChild without removeChild makes no difference.
  • Likewise scroll positions are the same, they always reset and you cannot avoid it by just using appendChild. Please see RFC New commit phase lifecycle getSnapshotBeforeUpdate() #33, it suggests a new API that solves the scroll issue for normal content updates and potentially could also be used to retain scroll state when reparenting. It may even work for retaining other states like focus and selection.

Conclusion

It would be nice to still note that React should try to use appendChild without first using removeChild if it does not need to. However this actually isn't a big deal. In practice it appears that almost nothing is lost during removeChild that is not lost during appendChild (as long as you re-insert within the same tick). The only difference I could find is that IE11 will pause the video when you removeChild+appendChild but not appendChild. But this is a pretty pointless difference because Chrome pauses the video on even a basic appendChild, so you are going to have to make the playing state a controlled behaviour anyways.

Results

Chrome append remove+append delay
video pauses, but doesn't lose position pauses, but doesn't lose position pauses, but doesn't lose position
audio pauses, but doesn't lose position pauses, but doesn't lose position pauses, but doesn't lose position
iframe refreshes refreshes refreshes
scroll loses scroll position loses scroll position loses scroll position
Firefox append remove+append delay
video does not pause or lose position does not pause or lose position pauses, but doesn't lose position
audio does not pause or lose position does not pause or lose position pauses, but doesn't lose position
iframe refreshes refreshes refreshes
scroll loses scroll position loses scroll position loses scroll position
Safari append remove+append delay
video does not pause or lose position does not pause or lose position pauses, but doesn't lose position
audio does not pause or lose position does not pause or lose position pauses, but doesn't lose position
iframe refreshes refreshes refreshes
scroll loses scroll position loses scroll position loses scroll position
IE11 append remove+append delay
video does not pause or lose position pauses, but doesn't lose position pauses, but doesn't lose position
audio could not test with .wav could not test with .wav could not test with .wav
iframe refreshes refreshes refreshes
scroll loses scroll position loses scroll position loses scroll position

@dantman
Copy link
Author

dantman commented Mar 15, 2018

Something else that is hard to test is selection behaviour. This probably isn't an issue for overall selection (I notice that even when completely removing and creating dom nodes a selection outside of them will update to contain even whole block nodes). However it may be relevant for reparenting large paragraphs with some selected text in them or large textarea inputs with a selection being edited.

@dantman
Copy link
Author

dantman commented Mar 15, 2018

Important discussion points:

Is it possible to hydrate a server-rendered DOM tree and know, without needing deterministic IDs, and know what portion of it belongs to a Reparent and should be detached and held on to when unmounting instead of destroyed? (Or would we need to wait for the internals of #32 to solve this)

Should the reparent function (this.header(children)) accept multiple children with a spread, the same way createElement accepts multiple children? These would simply be passed on to createElement, so the Fragment could contain more than just one child. Or is this API confusing and we should just recommend people needing this pass in a Fragment as the child of the Reparent?

When a Reparent's tree is detached from the DOM (the DOM nodes are not part of the document, but the tree still exists so it can be re-inserted later) and the Reparent is given new children. Should it render those children into the detached tree; or, should it just store the new children and wait until the tree is re-mounted before rendering them.

I can see rationales for both:

  • If we render to a detached tree it should mostly work out, but some dom operations that may be done in refs may not like it and expect there to be a parent. Perhaps something like .closest that tries to crawl up to the root or dom operations that calculate layout.
  • If we don't render to a detached tree then React components inside that tree will not be updated. If this includes a props change that affects something like Redux/Flux subscriptions the Component may continue to work with out of date subscriptions, which could result in runtime errors as the Component tries to access data that doesn't exist anymore or is under a different name now.

@j-f1
Copy link

j-f1 commented Jun 17, 2018

@philipp-spiess What I was thinking is that if you stopped rendering the <Reparent />, it would cache the rendered node somewhere to reuse when you rendered it again.

@dantman
Copy link
Author

dantman commented Jun 17, 2018

The ability to update the contents of a reparent is one of the key pieces of functionality we are going for. But also that proposal would permanently leak memory since it doesn't have a way to dispose of the reparent when you are no longer using it.

@aapoalas
Copy link

aapoalas commented Jun 17, 2018

@philipp-spiess I was thinking something along the following lines (though thinking deeply on this I keep finding more and more problems so this isn't going to be a whole lot of good I think):

I would have createReparent return a unique React Element that acts similarly to a React Fragment; inside it you can render any Components and the contents is diffed normally for changes. Each time you render it, the contents must be rendered as well: No keeping of references that remember and render what the previous contents was, or some such.

The React.Detached Component would again be similar to React Fragment; inside it you can render any Components and the contents is diffed normally for changes. Yes, I mean you could also render normal divs etc. inside this component. It would act the same as creating a DOM tree manually without appending it to be a child of the body. This would, however, be mostly useless without the Reparent Component, as the moment you "move" your normal DOM elements from within the React.Detached to some actual DOM tree, the Components of course unmount and are destroyed, and new ones created in the new location. The Reparent Component could however use the React.Detched to keep itself alive while detached without ill effects.

EDIT: React.Detached could be created in userland more or less as follows:

React.Detached = elem => {
  const target = document.createElement("div");
  return ReactDOM.createPortal(elem, target);
}

So the usage of these would be something like:

const ReparentA = React.createReparent(); // This would more likely be created inside some parent component during mounting or updating based on some inflowing data

class MyPointOfForking extends React.Component {
  render() {
    const {
      children,
      condition,
    } = this.props;
    return (
      <ForkA>
        {condition === true && <ReparentA>{children}</ReparentA>}
      </ForkA>
      <ForkB content={condition === false && <ReparentA>{children}</ReparentA>} />
      <React.Detached>
        {typeof condition !== "boolean" && <ReparentA>{children}</ReparentA>}
      <React.Detached>
    );

So we have some React tree (children in this case, but could just as well be created inside the render function) that we want to render somewhere, but the location changes based on some condition, or it can be detached from the DOM entirely in some cases.

Thus we have two choices by and large: Either we place the ReparentA Component (remember, this is a unique individual) in a normal React DOM tree one way or another (ForkA and ForkB, where ForkA will render directly as children, while ForkB might likely pass the content prop onwards to somewhere even deeper, before it finally gets rendered as a child), OR we can render the component in a detached React DOM tree.

Doing more than two of these will cause an error. You cannot render a Reparent in two places, not even if one of those is a Detached DOM tree.

I can see some problems with this approach though:

  • Keeping the Reparents straight (which one should render what content etc) can easily become a chore and might well end up detaching and remounting your contents even thought the whole point is to avoid that.
  • Having a unique Component is heretofore unknown in React and will indubitably cause confusion.
  • The need to keep the contents / children of each Reparent in order will likely lead to passing of the Reparent Components as props or children through React Component trees, and much deeper than is done currently. This is not necessarily a good thing, though both the Context API and Portals can be of help here.

Some good things:

  • The lifecycle of a Reparent component is essentially identical to that of a normal component; if at some point the component fails to be returned from some render function, it will unmount.
  • Although it may seem so, this approach does not mind rendering the pretty much anywhere; even though the Reparents are likely created within some parent component, f.ex. based on an array of ids, the parent component does not tie its own lifecycle methods to the Reparents. This should work fine as long as you do not attempt to render a Reparent as a parent of the component that originally created the Reparent, as that would lead to a constant creation of a new Reparent that would then become its own parent, triggering a new creation...
  • A Reparent that can be rendered at present DOM node or in some deep child can quite easily be made using the Reparent and Context API. Use Context to pass on either null or the Reparent component, while rendering the other at present node.
  • A Reparent that can be rendered at present DOM node or in eg. a modal container can quite easily be made using the Reparent and Portal API. Render the Reparent component either normally, or into your modal container using a Portal.
  • A Reparent that can be detached from the DOM at will is as easy as the above Portal case. Just use React.Detached instead of React.Portal (which React.Detached is just a special case of, essentially, and could easily be created in userland)

@dantman
Copy link
Author

dantman commented Jun 17, 2018

Doing more than two of these will cause an error. You cannot render a Reparent in two places, not even if one of those is a Detached DOM tree.

The ability to "keep" a reference while still rendering it elsewhere is a requirement. This API won't work well when you have to wait for something async to finish when moving a reparent from one child to another.

@aapoalas
Copy link

I don't quite understand what kind of reference do you want to keep, and what would you want to do with it.

Also, what do you mean by "wait[ing] for something async to finish when moving a reparent from one child to another"? Do you mean that part of the content of a reparent will move first, and some other part will be moved later once some async action has finished?

@dantman
Copy link
Author

dantman commented Jun 18, 2018

By keep a reference I mean it should be possible for a component to use a reparent such that it will not unmount, without providing children, and while still allowing another component to actually render the reparent.

By waiting for async, I'm referring to a scenario where a parent with dynamic contents passes reparents to children (likely using context), the children render the reparents, then the dynamic content changes. If the dynamic content changes such that a reparent immediately moves from one child to another child then the reparent would normally move fine. However, if the second child happens to need to wait for say some API call to fetch before it can render then the reparent will not have a destination until that is fulfilled. If the parent cannot essentially say "this reparent is still in use, even if no-one is currently rendering it" then while we are waiting for the API call to finish the reparent will end up unmounting instead of detaching so it can be inserted into the second child when it renders.

@aapoalas
Copy link

Reference: What does the first component "use" the reparent for if it does not provide content for the reparent, and does not render the reparent?

Waiting for async: In my opinion this is simply a question of keeping proper tabs on the reparent components, just as you would keep proper tabs on components you create in normal renderers. eg. The second child needs to wait for some API call before it is ready to actually render the reparent. Then in its renderer it should return <React.Detached>{reparent}</React.Detached> until the API call is done, after which it will render some other stuff along with the reparent. Or alternatively the parent should keep tabs (via Redux store, or just callback functions, or w/e) on which reparent components are actually in use, and itself return the rest inside a React.Detached Component.

If it really is thought necessary to allow for the parent to essentially call keep() on reparents it has created just "to be sure they don't disappear", then I believe we need to bring back the this-binding and maybe even explicit unmounts (I've reversed my stance on .keep()-function inside render :)).

@philipp-spiess
Copy link

There will still be problems with Async React. In the future, React might render and commit one part of the tree before the other part renders (see this tweet from Dan). This means that there might be a "hole" when no reparent is rendered although you're immediately moving it into a different subtree.

This is why we need an API where a shared controller can tell React to not GC certain reparents - No matter if they appear in a commit phase or not.

@dantman
Copy link
Author

dantman commented Jun 18, 2018

Reference: What does the first component "use" the reparent for if it does not provide content for the reparent, and does not render the reparent?

The parent knows that the item the reparent is associated with still exists. It does not know if it will be rendered.

Waiting for async: In my opinion this is simply a question of keeping proper tabs on the reparent components, just as you would keep proper tabs on components you create in normal renderers. eg. The second child needs to wait for some API call before it is ready to actually render the reparent. Then in its renderer it should return <React.Detached>{reparent}</React.Detached> until the API call is done, after which it will render some other stuff along with the reparent. Or alternatively the parent should keep tabs (via Redux store, or just callback functions, or w/e) on which reparent components are actually in use, and itself return the rest inside a React.Detached Component.

  • Assuming the parent knows enough about the async request, this just adds needless complexity trying to track this when there is no need to track it
  • The child returning a detached container works if the child is the one doing the network request, but if the async request comes from an intermediate component "Parent > AsyncElement > Child" neither of them know when they should render a detached node in order to just avoid the component being unmounted.

I've reversed my stance on .keep()-function inside render :)

How about the alternative JSX based api? That uses a <Reparent.Reference />.

@aapoalas
Copy link

There will still be problems with Async React. In the future, React might render and commit one part of the tree before the other part renders (see this tweet from Dan). This means that there might be a "hole" when no reparent is rendered although you're immediately moving it into a different subtree.

I do not know much anything about Async React, having already forgotten all that was said in the presentation. I believe this is not an abyss we should look too deeply into. Dan mentions avoidance of "tearing" and if reparents were a current part of React, I'm sure reparents losing state / unmounting would be considered "tearing". He also makes a vague reference to the postponing of commission being an opt-in thing, so such a situation where a reparent gets unmounted due to postponed rendering could also be argued to be a user error.

The child returning a detached container works if the child is the one doing the network request, but if the async request comes from an intermediate component "Parent > AsyncElement > Child" neither of them know when they should render a detached node in order to just avoid the component being unmounted.

Mmm... I'd want to say that the AsyncElement here should take the reparent from the context and either render it in React.Detached, or pass it as a prop to Child once it is done with the necessary request. Similar to lifting of state and all that. Of course, if AsyncElement here is some 3rd party component that you can't mess around with, you may be in quite some trouble.

How about the alternative JSX based api? That uses a <Reparent.Reference />.

... It's okay, maybe. I'm not exactly thrilled with it, as I already managed to sell myself on my idea. (Surprise!) I've managed to convince myself that a as close to a normal element as possible is the prettiest and most elegant solution and thus the best. So having this extra Reference thing is weird as it doesn't really have any precedent and smells a bit of laziness ("I don't know if my stuff is actually needed, so I'll just keep it around just to be safe...").

The whole Reference / keep() -system feels a little like tying the <Reparent> into the parent's lifecycle methods without actually doing it. So in that sense if what is wanted is a type of reparent that is created and stays in existence until it is explicitly thrown away (leaving out the Reference is an explicit act), then I'd maybe rather just have <Reparent> automatically become detached and stop updating if it is not rendered anywhere, and bring back the explicit unmount(). This would remove the need for React.Detached and Reference and would still allow reparents to be relatively normal components.

Throwback:

We do need to allow a reparent to be rendered with empty children.

Why? What would an empty reparent accomplish? Except work as a different kind of null. How I see it in my proposal, once a reparent is created it can be used, become unmounted and be reused later if that is wanted, for some reason or another. Of course all components inside it would likewise be gone, but that's a designed for and intended facet of my proposal.

Rendering the reparent as empty would, of course, work as well but it would simply not do anything nor would it be a necessary thing in order to keep the reparent usable for a future case. I can only see this being a reasonable situation when the only child (or all children) of a reparent return nulls from their renderers, in which case the reparent is empty, DOM-wise, but not React-wise.

@dantman
Copy link
Author

dantman commented Jun 18, 2018

I do not know much anything about Async React, having already forgotten all that was said in the presentation. I believe this is not an abyss we should look too deeply into. Dan mentions avoidance of "tearing" and if reparents were a current part of React, I'm sure reparents losing state / unmounting would be considered "tearing". He also makes a vague reference to the postponing of commission being an opt-in thing, so such a situation where a reparent gets unmounted due to postponed rendering could also be argued to be a user error.

From what I've seen of the suspense api so far. It's encapsulated enough that forcing a parent to know enough about what it's children are rendering in order to know when it needs to render a detached node in some apps would break encapsulation. And a reparent unmounting because a placeholder was rendered temporarily is not user error.

Additionally while suspense is great, it's not the only way to do async react. Apps are currently already doing async react using componentDidMount and state. Even if suspense were to somehow solve the reparent issue internally and avoid unmounting something that may be used in the tree it is waiting for; it still wouldn't solve the issue for all the apps currently not using suspense for fetching data.

Mmm... I'd want to say that the AsyncElement here should take the reparent from the context and either render it in React.Detached, or pass it as a prop to Child once it is done with the necessary request. Similar to lifting of state and all that. Of course, if AsyncElement here is some 3rd party component that you can't mess around with, you may be in quite some trouble.

Even if we assume AsyncElement wasn't a 3rd party component, pretend it was say a lightweight component tasked with downloading i18n data and then rendering its children once that data is available. It would be a pretty big encapsulation break to say that knowledge of Child's reparent should be hoisted into AsyncElement.

And frankly even the fix of having the Parent pass a "render this if you aren't going to render Child at the moment" down through the tree gets pretty messy as you try to add layers.

Why? What would an empty reparent accomplish? Except work as a different kind of null. How I see it in my proposal, once a reparent is created it can be used, become unmounted and be reused later if that is wanted, for some reason or another. Of course all components inside it would likewise be gone, but that's a designed for and intended facet of my proposal.

My issue was primarily with the distinction between <Reparent>Foo</Reparent> being the way that one renders to a reparent. And <Reparent></Reparent> ending up being a reference that keeps the children and should usually be used in <React.Detached>.

A) React allows you to render an element with no children, and the only difference is the absence of the data in the children prop. It would be a pretty large break from React conventions if at a fundamental level <Reparent>Foo</Reparent> rendered 'Foo' and <Reparent></Reparent> kept rendering 'Foo' instead of removing it.
B) children can also be undefined as a result of a conditional, <Reparent>{condition && <Bar />}</Reparent>. You would expect this to render <Bar /> when condition is true <Foo condition /> and nothing when condition is undefined (remember <Foo /> has an undefined condition, not false). Surprise! It actually renders <Foo /> when true, and when undefined it's actually a reference which would suggest that the reparent is still rendered but its children are not modified.

The whole Reference / keep() -system feels a little like tying the into the parent's lifecycle methods without actually doing it. So in that sense if what is wanted is a type of reparent that is created and stays in existence until it is explicitly thrown away (leaving out the Reference is an explicit act), then I'd maybe rather just have automatically become detached and stop updating if it is not rendered anywhere, and bring back the explicit unmount(). This would remove the need for React.Detached and Reference and would still allow reparents to be relatively normal components.

It actually feels like the reverse for me. I think of it similar to GCing. React doesn't unmount things as an explicit order from someone to unmount something, it unmounts things implicitly when they are no longer in use anywhere. Just like how a GC tracks what objects are being used and removes any objects that are no longer referenced.

Having to explicitly track and .unmount() reparents sounds like adding an imperative order, in a place where React doesn't work imperatively. While .keep() and <Reparent.Reference /> are declarative "I'm still using this" declarations. And they fit in with the GC-like behaviour of unmounting by declaring when you are still using something (have a "reference" to it like the GC tracks references) and letting React unmount it when you no longer indicate it is in use.

Aside from that, people brought up some legitimate concerns about the explicit .unmount() API in that it's too easy to accidentally introduce a memory leak.

... It's okay, maybe. I'm not exactly thrilled with it, as I already managed to sell myself on my idea. (Surprise!) I've managed to convince myself that a as close to a normal element as possible is the prettiest and most elegant solution and thus the best. So having this extra Reference thing is weird as it doesn't really have any precedent and smells a bit of laziness ("I don't know if my stuff is actually needed, so I'll just keep it around just to be safe...").

I was going to agree and arguer further that the idea of a unique instance <Reparent> element where Reparent is a dynamically generated element with it's own context was strange and didn't behave like a normal element any more than adding .Reference. But then I realized I was wrong. Uniquely generated elements and use of Foo.Bar is not strange or without precedent.

const FooContext = React.createContext();
// ...
<FooContext.Provider value='foo'>
  <FooContext.Consumer>
    {foo => <span>{foo}</span>}
  </FooContext.Consumer>
</FooContext.Provider>

React's new context system's Provider and Consumer are unique component instances where the component itself is tied to the context of the FooContext. And they also are commonly used in the Foo.Bar format.

The only modification I need to make to my suggested API to make it behave exactly like the context API is to make it so that you don't use Reparent directly as an element.

const Reparent = React.createReparent();
// Keep a reference to the reparent, without changing its contents.
// Can be used as many times as you want.
<Reparent.Reference />
// Render something into the reparent's context
// and render the reparent into an element.
// Can only be rendered in one location at a time.
<Reparent.Scope>...</Reparent.Scope>
// Render a reparent into a detached state.
// This is distinct from just using `<Reparent.Reference />` to keep a reference
// to avoid the component unmounting and it detaching when it's no longer rendered.
// The distinction is that in this case you are explicitly updating the
// reparent's contents while it is detached.
<React.Detached>
  <Reparent.Scope>...</Reparent.Scope>
</React.Detached>

All three of these terms are up for better naming suggestions.

@aapoalas
Copy link

The only modification I need to make to my suggested API to make it behave exactly like the context API is to make it so that you don't use Reparent directly as an element.

Yes, this is good! Although I'd at least give thought to <Reparent.Reference> having to encapsulate the React tree within which <Reparent.Scope> will be used like how it works with Context API. This might end up with a distinctly weird, deep tree of <Reparent.Reference>s when an array of reparentable components are created, but would allow for control over where the Reparent might actually be returned from.

Overall, though, I think this might well be the True Form of the API 👍

Sidenote: I can't remember too well how <Placeholder> was meant to work and if it would break this idea, but I was imagining a reparent unmounting tied to "synchronized" render states; that is to say that if a reparent component was removed from the React tree and did not appear elsewhere, then the unmounting of said reparent would be delayed until the next complete, "unteared" render state is reached. This would cause delayed componentWillUnmount calls from reparents in Async React applications but would otherwise be semi-ok. However, this does not offer any way for non-Suspense components to handle Reparents "natively".

@dantman
Copy link
Author

dantman commented Jun 18, 2018

Although I'd at least give thought to <Reparent.Reference> having to encapsulate the React tree within which <Reparent.Scope> will be used like how it works with Context API.

Interesting idea. But no good, that would result in this.

const ReparentA = React.createReparent();
const ReparentB = React.createReparent();
const ReparentC = React.createReparent();

<ReparentC.Reference>
	<ReparentB.Reference>
		<ReparentA.Reference>
			<Foo />
		</ReparentA.Reference>
	</ReparentB.Reference>
</ReparentC.Reference>

Doing this dynamically would be a mess. If your dynamic structure uses a list of dozens of children, then the DevTools tree would be dozens of elements deep for everything. And ironically, unless you make use of an additional <HostReparent.Scope> the entire tree outside the reparents will remount every single time you add/remove a reparent. 😱🤣

@aapoalas
Copy link

that would result in this

Yeah, I foresaw this. It would be so nice to look exactly like the Context API but it definitely is better without doing so.

@philipp-spiess
Copy link

Sorry for throwing in the confusion about async React. I thought more about it and this is an implementation detail and not a point for deciding about the API.

👍 for the latest JSX API - I guess the <React.Detached/> component is only for demonstration purposes and not part of this RFC though, correct?

Here are some alternative name ideas for the <Reparent.Reference /> component:

  • <Reparent.Keep />
  • <Reparent.KeepAlive />
  • <Reparent.Persist />
  • <Reparent.KeepLatest />
  • <Reparent.NoUnmount />

And here are some ideas for <Reparent.Scope/>:

  • <Reparent.Target />
  • <Reparent.Content />
  • <Reparent.Inside />
  • <Reparent.Reparentable /> 🙃

My favorites so far are <Reparent.Persist /> and <Reparent.Content /> but I urge you to just take this as additional input with no strings attached as I'm terrible at finding good names and always go for the "make it easy to grep and change later" solution 🙈

@dantman
Copy link
Author

dantman commented Jun 19, 2018

👍 for the latest JSX API - I guess the <React.Detached/> component is only for demonstration purposes and not part of this RFC though, correct?

React.Detached would be included since I brought up the use case of rendering a reparent's children while it is detached. And this functionality is useful in non-dom scenarios as well so it makes sense for Detached to not be a ReactDOM/custom thing.

@philipp-spiess
Copy link

React.Detached would be included since I brought up the use case of rendering a reparent's children while it is detached. And this functionality is useful in non-dom scenarios as well so it makes sense for Detached to not be a ReactDOM/custom thing.

I'm not sure if the use case is really as pressing that it requires a new React top level API that is probably expected to work with regular elements as well but that will be completely useless unless a reparent is used (since React will throw away the DOM nodes otherwise anyway).

() => (
  <React.Detached>
    <p>Why would I want to do that?</p>
  </React.Detached>
);

There might be a reason for updating the detached reparents as well (although I'd say that usually it is OK to only update when it becomes attached again) but I think this can be done within a <div style={{display: none}} /> as well (or a RN alternative). I don't think that this is a pattern we need to encourage.

I believe that the smaller the API surface, the more likely the React team will approve it.

@aapoalas
Copy link

How're we doing on this RFCS? @dantman Is there anything that can be helped with?

@philipp-spiess
Copy link

@dantman What do you think about replacing this PR with a newer one that incorporates all the issues discussed? I think this will make the RFC much more approachable since the discussion here is already very long. I remember it took me quite some time to read through all that and we've since doubled the comments 😐

Let me know if I can help you somehow!

@falconmick
Copy link

Sorry for being that person, I have not read all of the comments. However I wanted to include a usecaes:

I have a React component that I need to transition from a list of posts that is animating, into a React portal which is outside of the

to avoid position: absolute and transform issues, then it will transform the location to a new spot in the page (the top full width) and re-attach itself to the react dom again.

Basically I'm trying to make the image transition effect you get in allot of IOS/Android apps.

@benwiley4000
Copy link

benwiley4000 commented Dec 3, 2018

Does this RFC suggest that React would move around the actual DOM nodes? Is it technically possible? For example, if one of the child node was a playing <video> element, by removing it from DOM and reinserting into a different parent, would it still be playing?

IMO it should just move the element and leave the developer to do whatever they would do. For what it's worth, the behavior on moving a video element is clearly defined in the HTML spec (https://html.spec.whatwg.org/multipage/media.html#media-playback):

When a media element is removed from a Document, the user agent must run the following steps:

  1. Await a stable state, allowing the task that removed the media element from the Document to continue. The synchronous section consists of all the remaining steps of this algorithm. (Steps in the synchronous section are marked with ⌛.)

  2. ⌛ If the media element is in a document, return.

  3. ⌛ Run the internal pause steps for the media element.

Note that the element is only paused if it was completely removed from the DOM (i.e. not moved). However in my tests in Firefox and Chrome I found that Firefox follows the spec (continues playing the video) while Chrome violates the spec (pauses regardless of the destination of the element). Nothing is mentioned about currentTime.

I'm filing a bug report for Chromium about this: https://bugs.chromium.org/p/chromium/issues/detail?id=910988

So to summarize:

According to the spec, video should continue uninterrupted when moved around the DOM, but at worst, you might need to call .play() again. This is a compelling use case to support re-parenting in React.

@dantman
Copy link
Author

dantman commented Feb 4, 2020

😆Coming back to this years later, I'm surprised that my proposed API still holds up. I thought I might need to revise it to work with hooks. But it looks like the proposal still works for functional components with hooks. Only perhaps with a user land helper for creating a singular reparent within a component.

It's hard to sift through the comments and figure out what revisions I accepted. But I am thinking of reversing some of the ones I might have accepted.

There was a lot of discussion about a JSX style API. It seems I accepted and got into this idea because of the similarity to the context render props API. However now that Context.Consumer has become something rarely used, with useContext(Context) being more common I'm rethinking it. The "declarative magic" of tracking renders where .keep() is used is no longer strange now that hooks do effectively that.

I'll need to sleep on some idea of what a reparenting API that fits in with hooks would look like.

@dantman
Copy link
Author

dantman commented Aug 22, 2020

The most compelling argument for reparenting IMHO is page layout. i.e. The ability to write route components that all use a shared <Layout pageTitle='About' breadcrumbs={['Home', 'About']}> component without the entire page layout being unmounted and recreated on page transition.

How this fits into concurrent mode should probably be thought about. And what the most user friendly API for this use case would be should be thought of.

@ragnaroh
Copy link

ragnaroh commented Jan 1, 2021

This might be a stupid question (I'm not familiar with the inner workings of React), but couldn't reparenting be solved by introducing a globalKey attribute (akin to the key attribute), which must be unique across all components in the component tree? Then upon re-rendering, React would compare the global keys of the previous tree to the global keys of the new tree, and if any keys have "changed locations" in the tree, move the DOM nodes accordingly (as well as internal references to the components).

@gaearon
Copy link
Member

gaearon commented Aug 18, 2021

So, there is a lot to unpack here. As requested in #182, I'm making a pass to review the things that I've been hesitant to review because a proper review would take a very long time to write. So I'll instead leave some cursory comments.

Reparenting is one of the Hard Problems(tm). It basically affects all features at the same time. So the bar for a proposal would have to be extremely high. I think over time we've come to realize that "true reparenting" in all senses of the word usually isn't really needed, and can be a distraction. Here's a few concrete scenarios, and what we tend to use instead:

  • "Detachable video player". That seems like a canonical use case for reparenting. E.g. you start watching a video inside a tweet, then you scroll down but the video detaches and keeps playing. Intuitively this seems like a case for reparenting. However, it's not quite clear-cut if you think about what should happen to Context. I think you probably want the Context from the place where the video was detached. Not from the top level where it got reparented to. You might also want to keep receiving prop updates from the original place in the feed, e.g. if the like count changes. I know people like to say "UIs are graphs, not trees" but I really don't know that this is actually true. Anyway, for cases like this, it seems like the desired effect is visual reparenting / DOM reparenting while the React tree is intact, and the component stays in its original place. This is clumsy but already possible today. The way you do this is by combining refs and portals. The "tweet" (original parent) uses a ref to a DOM node, and places another DOM node into it. It portals the "video" (the child) into that second DOM node. Then, you can imperatively move that second DOM node anywhere in the DOM tree when needed, including reattaching it back. The effect is purely visual, and the actual data flow (and the React tree) stays the same, so there are no difficult questions to resolve. While React could provide a nicer way to do this, it's already possible to do today, even with a pretty userland API wrapper.
  • "Route transitions". There are actually two different cases.
    • "Compatible transitions". The example in Reparenting API #34 (comment) mentions page layout. But that already works in React, in general. It only doesn't work if your framework forces you to declare route pages as Home, About, etc, which use Layout inside. But a more React-y way should rely on reconciliation directly. I think this says that routes shouldn't exactly be components, since we really want the Home page and the About page to be able to "line up" because they contain a Layout. This is not a purely visual problem: we want to maintain the state (e.g navigation bar hover, search state) as well. Bringing in reparenting here is an overkill when React can already handle reconciliation just fine. It's just that we need a routing abstraction that doesn't opt you out of proper reconciliation by default.
    • "Magic transitions". Sometimes, there really isn't a way to transition properly. Like actually different layouts. This is usually about animations specifically. However, in this case you often want to do it to completely different components anyway. There's no way to line them up. It's more of a layout animation or "magic move" use case where the source and the target might even be written by different people. Framer Motion does a version of this, and building some first-class support for this would be in scope of a React Animations project that we hope to get to in the future. However, that's not really related to the traditional React tree reparenting.

Now, this doesn't mean that reparenting is completely useless. There might be some use cases I've missed. But in many of these, a combination of the above four approaches together with more traditional approaches like lifting state up, is very likely sufficient. On the other hand, reparenting within the React tree raises so many deep questions that it's hard to justify the complexity.

@depoulo
Copy link

depoulo commented Aug 19, 2021

It's just that we need a routing abstraction that doesn't opt you out of proper reconciliation by default.

Now someone please tell me that's out there already and I don't have to write it myself.

@lewisl9029
Copy link

It's just that we need a routing abstraction that doesn't opt you out of proper reconciliation by default.

Now someone please tell me that's out there already and I don't have to write it myself.

router5 fits the bill I believe: https://router5.js.org/

I've always been a fan of its routing as state approach over the traditional routing as entry points approach taken by most client side routers. router5's author eloquently explains in his talk introducing router5, that routing as entry points is a pattern that was blindly adapted from server side routing, which is mostly stateless and transactional (i.e. an API request will only ever result in 1 route branch being exercised for the lifetime of the request), while client side routing is inherently stateful and dynamic (i.e. an SPA will need to keep track of its "current" route and often transition between many different routes and nested routes in a single browsing session), which is where much of the impedance mismatch comes from.

In more concrete terms, an app built with router5 would just have plain react components subscribing to routing state changes and branching accordingly with plain if/else/switch statements and/or map look-ups (this is the actual "routing" part) at any arbitrary depth in the component tree, instead of relying on special snowflake components (i.e. Routes and Routers) at the highest level of hierarchy responsible for branching between routes and rendering everything downstream.

It really is a shame that development on the project seems to have stalled. The concept itself is really simple though. I've been using a few custom routing utils in my own projects based on the same principles for a while. The entire routing logic fits in less than 100 lines and is super easy to reason about. Lately I've also been experimenting with adding some aspect of relative routing introduced by react-router v6 (IMHO that's one of the only redeeming ideas in there unfortunately, as the rest of it seems to be doubling down on routing as entry points), which so far seems to be a great fit but it will need a bit more battle testing.

@itayperry
Copy link

Any progress on this issue?

The "tweet" (original parent) uses a ref to a DOM node, and places another DOM node into it. It portals the "video" (the child) into that second DOM node. Then, you can imperatively move that second DOM node anywhere in the DOM tree when needed, including reattaching it back.

saving refs so many levels deep sounds like the wrong way to go - I think it would be better to actually have an API for that.

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.