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

[Fiber] Add ReactDOMFiber.unstable_createPortal() #8386

Merged
merged 2 commits into from
Nov 22, 2016

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Nov 22, 2016

While #8368 added a version of ReactDOM.unstable_renderSubtreeIntoContainer() to Fiber, it is a bit hacky and, more importantly, incompatible with Fiber goals. Since it encourages performing portal work in lifecycles, it stretches the commit phase and prevents slicing that work, potentially negating Fiber benefits.

This PR adds a first version of a declarative API meant to replace ReactDOM.unstable_renderSubtreeIntoContainer(). The API is a declarative way to render subtrees into DOM node containers.

It looks like this:

function WelcomePopup() {
  return <h2>Welcome</h2>;
}

function App() {
  return (
    <div>
      <h1>Hi</h1>
      {ReactDOM.unstable_createPortal(
        <WelcomePopup />,
        document.getElementById('popup-container')
      )}
    </div>
  )
}

Fiber treats it as a regular slice-able work and commits the subtrees according to depth, like normal components. Context updates "just work" because of this. I copied the subtree tests, modified them to use the new API, and added them to ReactDOMFiber-test since it's a Fiber-specific feature. I also added some more assertions around the lifecycle order and ensuring node removal.

There are a few missing pieces:

  • Only DOM node is accepted as a container. I'm not sure if we should accept a () => node instead because node might not be ready yet. This also doesn't play with server rendering, but since the support is baked into reconciler we can add ReactDOMServer.createPortal(element, stream) someday (note: not an actual API, I’m just showing potential future directions as discussed with @sebmarkbage).

  • The portal object has a reserved implementation field that is currently unused. It will be used for supporting cross-renderer portals (e.g. ART inside DOM). It will contain { beginWork, completeWork, commitWork } or something similar of the nested renderer. The parent renderer will still be responsible for scheduling, but will forward work to a nested renderer when it's inside a portal.

  • We will need to keep track of the roots on the stack so that we can fix document.body hack in ReactDOMFiber.

  • We still use updateContainer which is hacky since it replaces all children. The plan is to eventually move the output traversal to the complete phase, and let the renderer provide *ToContainer overloads of appendChild, insertBefore, and friends.

This is merely a first step towards supporting portals in Fiber, and it is not complete, but I want to land it so that we have the basics (like we do with coroutines) and don't regress on them.

Next I will work on SVGs and try to reuse this primitive for them.

While facebook#8368 added a version of `ReactDOM.unstable_renderSubtreeIntoContainer()` to Fiber, it is a bit hacky and, more importantly, incompatible with Fiber goals. Since it encourages performing portal work in lifecycles, it stretches the commit phase and prevents slicing that work, potentially negating Fiber benefits.

This PR adds a first version of a declarative API meant to replace `ReactDOM.unstable_renderSubtreeIntoContainer()`. The API is a declarative way to render subtrees into DOM node containers.
@sebmarkbage
Copy link
Collaborator

cc @bvaughn for possible React ART use case.

@ryanflorence
Copy link
Contributor

Love it. I've always wanted to call this.renderPortal() inside of render anyway.

Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

Choose a reason for hiding this comment

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

Accepting with some minor nits comments.

export type ReactNode = ReactElement<any> | ReactCoroutine | ReactYield | ReactText | ReactFragment;
import type { ReactPortal } from 'ReactPortal';

export type ReactNode = ReactElement<any> | ReactCoroutine | ReactYield | ReactText | ReactFragment | ReactPortal;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you place ReactPortal before ReactText and ReactFragment? It used to matter. I'm not sure it still does but there was something weird with ReactEmpty before. I think I had a similar comment in another PR.

@@ -63,7 +64,11 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
// If we have a single result, we just pass that through as the output to
// avoid unnecessary traversal. When we have multiple output, we just pass
// the linked list of fibers that has the individual output values.
returnFiber.output = (child && !child.sibling) ? child.output : child;
if (!child || child.tag === Portal) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we let Portal always have a null output we don't need to do this.

@@ -251,6 +256,10 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
case Fragment:
transferOutput(workInProgress.child, workInProgress);
return null;
case Portal:
transferOutput(workInProgress.child, workInProgress);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's make this always leave output as null and just use this fiber as is in commit work instead.

@@ -291,6 +297,12 @@ module.exports = function<T, P, I, TI, C>(
commitTextUpdate(textInstance, oldText, newText);
return;
}
case Portal: {
const children = finishedWork.output;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can just do const children = finishedWork; for now since the host config will just traverse the tree deeply right now.

@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Nov 22, 2016

For context: We might also consider an isomorphic API like React.createPortal(element, selector) where selector is a string in the future where the renderer can determine whether to send it to a separate stream (on the server) or a querySelector (on the client). It's a bit more complex since layer solutions require some coordination logic that might be context specific anyway.

@sebmarkbage
Copy link
Collaborator

Can we add a test for event bubbling through a portal? I don't know what the semantics currently are but we should assert what happens. I think it will bubble through to the parent of the portal (the thing that renders the portal).

https://github.com/facebook/react/blob/master/src/renderers/shared/shared/ReactTreeTraversal.js#L23-L25

We should also test what happens if a different React tree is a parent of the portal target itself. I think what currently happens is that it will not bubble up to that tree.

@gaearon
Copy link
Collaborator Author

gaearon commented Nov 22, 2016

Can I land this and add tests in a followup?
I won't be available tomorrow and I don't want to have to rebase this later.

@gaearon
Copy link
Collaborator Author

gaearon commented Nov 22, 2016

I'll land because going to bed. Yolo!

@lexfrl
Copy link
Contributor

lexfrl commented Dec 8, 2016

This is good, that you're working on this "portals" problem. There is a lot of popover/modals/tooltip components could be found in the web just because of lack of good for "render in another branch" API in React.. This is because, there is no ""default" way, how to mount component to the different branch (by some id, for example) of the components tree.

I've done some research on generalization of "portals", or, better "modules" problem. In its core idea is to have some "mountpoints" with IDs, just like soft/symlinks (to directories) in filesystems. Thus, user can render some component (like dialog or tooltip) with its local context right into the different branch it the tree. Also user can choose b/w "mountpoints" to mount to (which could represent different panels or it could be some fixed layer (for modal window or popup) for instance). The idea is to hold these bits of information in a "module":

  1. local lexical context (table cell ID, for instance)
  2. state of the "module" (is active of not, arguments)
  3. callbacks to show(with arguments)/hide/toggle
  4. index (zIndex) or "order" inside of the MountPoint

As a generalization, "module" could be exposed as a first class citizen, like a Component. React application itself could be exposed as a "module", which contains another "modules" inside (which could be used as a separate applications as well). Module can link with another module by ID (these IDs are app-level entities, not the global ones). Module (have app-level-unique ID) can be mounted to the MountPoint (also have app-level-unique ID).

As an example:

// ... for each `object` in array of `objects`
const modalId = 'DeleteObjectConfirmation' + objects[rowIndex].id
return (
    <Cell {...props}>
        // the layer definition. The content will show up in the LayerStackMountPoint when `show(modalId)` be fired in LayerContext
        <Layer use={[objects[rowIndex], rowIndex]} id={modalId}> {({
            hide, // alias for `hide(modalId)`
            index } // useful to know to set zIndex, for example
            , e) => // access to the arguments (click event data in this example)
          <Modal onClick={ hide } zIndex={(index + 1) * 1000}>
            <ConfirmationDialog
              title={ 'Delete' }
              message={ "You're about to delete to " + '"' + objects[rowIndex].name + '"' }
              confirmButton={ <Button type="primary">DELETE</Button> }
              onConfirm={ this.handleDeleteObject.bind(this, objects[rowIndex].name, hide) } // hide after confirmation
              close={ hide } />
          </Modal> }
        </Layer>

        // this is the toggle for Layer with `id === modalId` can be defined everywhere in the components tree
        <LayerContext id={ modalId }> {({show}) => // show is alias for `show(modalId)`
          <div style={styles.iconOverlay} onClick={ (e) => show(e) }> // additional arguments can be passed (like event)
            <Icon type="trash" />
          </div> }
        </LayerContext>
    </Cell>)
// ...

These ideas came to me while working on https://github.com/fckt/react-layer-stack (demo https://fckt.github.io/react-layer-stack/). It could give a good insight to understand what I mean by "modules".

acusti pushed a commit to brandcast/react that referenced this pull request Mar 15, 2017
* [Fiber] Add ReactDOMFiber.unstable_createPortal()

While facebook#8368 added a version of `ReactDOM.unstable_renderSubtreeIntoContainer()` to Fiber, it is a bit hacky and, more importantly, incompatible with Fiber goals. Since it encourages performing portal work in lifecycles, it stretches the commit phase and prevents slicing that work, potentially negating Fiber benefits.

This PR adds a first version of a declarative API meant to replace `ReactDOM.unstable_renderSubtreeIntoContainer()`. The API is a declarative way to render subtrees into DOM node containers.

* Remove hacks related to output field
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.

5 participants