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

Fragment refs RFC #97

Closed
wants to merge 3 commits into from
Closed

Fragment refs RFC #97

wants to merge 3 commits into from

Conversation

sebmarkbage
Copy link
Collaborator

Summary

This is an alternative to findDOMNode.

Basic example

function Foo({children}) {
  let fragmentRef = useRef();
  useEffect(() => {
    let domNodes = fragmentRef.current;
    // ...
  });
  return <Fragment ref={fragmentRef}>{children}</Fragment>;
}

Rendered text

@streamich
Copy link

streamich commented Dec 10, 2018

Will there be short syntax version?

<ref={fragmentRef}>

</>

@sebmarkbage
Copy link
Collaborator Author

@streamich No. There are already other cases where the Fragment full name is required such as <Fragment key={...}>...</Fragment>. This doesn't change with this proposal. Adding a short form of this has too many syntactical ambiguities. The correct mental model is that <> is a shorthand for <Fragment>.

@sag1v
Copy link

sag1v commented Dec 10, 2018

Thank you @sebmarkbage ! 🙏

The child nodes are always DOMNodes because it drills through any custom component and finds the DOM nodes

Maybe i'm not reading this well, but when you say DOMNodes, is that includes strings?

Based on your example:

const Child = () => "hi"

function Foo({ children }) {
  let fragmentRef = useRef();
  useEffect(() => {
    let domNodes = fragmentRef.current;
    // ...
  });
  return <Fragment ref={fragmentRef}>{children}</Fragment>;
}

----------

<Foo>
  <Child/>
</Foo>

Will it "catch" the Child's content?

Maybe my real question is, will fragment ref be able to support All capabilities findDOMNode gave us?

@gaearon
Copy link
Member

gaearon commented Dec 10, 2018

Maybe i'm not reading this well, but when you say DOMNodes that includes strings?

A text node is also a DOM node so yes.

Will it "catch" the Child's content?

Yes.

Maybe my real question is, will fragment ref be able to support All capabilities findDOMNode gave us?

Is there some capability that you feel is missing? Just to clarify, findDOMNode did not support strings. As far as I can tell this RFC lets you implement more things than findDOMNode let you.

@quantizor
Copy link

quantizor commented Dec 10, 2018

this RFC lets you implement more things than findDOMNode let you.

I think the biggest issue here is it's no longer possible to get a DOM node for an arbitrary instance ref. Sure you can put a fragment around a third party component and acquire some DOM node from it, but then it's guesswork and hacky workarounds to find the appropriate subtree within that parent.

This is particularly problematic for layered HOCs with interim DOM where the thing you actually want may be quite deep in the hierarchy.

@jeremy-deutsch
Copy link

jeremy-deutsch commented Dec 10, 2018

Is this meant to also cover some of the use cases of React.Children?

@TheSavior
Copy link

Can you share some thoughts around how this API will be the same or different for React Native

@sag1v
Copy link

sag1v commented Dec 10, 2018

@gaearon

Is there some capability that you feel is missing?

Not sure, for my personal use cases it seems to cover them (will need to dig deeper though )

Just to clarify, findDOMNode did not support strings

Strange, if you wrap a child that returns a string:
findDOMNode(this) will return the #text. maybe we have a different definition for the word support regarding this?

@jquense
Copy link

jquense commented Jan 4, 2019

this is awesome 👍 Definitely addresses most of the potential concerns i've had with deprecating findDOMNode

cc @taion


# Alternatives

The alternative to this API is simply to not add anything and instead encourage libraries to use custom elements as wrappers. E.g. `x-fragment`. These can use tricks like `display: contents` to play nicer into layouts. Ideally the DOM would support something like this first class.
Copy link
Member

Choose a reason for hiding this comment

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

Migration concerns mean this is probably infeasible, but something like a better state of the world would have been a convention like components exposing some prop like hostRef to enable their parents to access their host nodes, in an opt-in manner.

So in that sense, a new API isn't strictly necessary, if we envision "access to host node" as an opt-in thing; but with my library maintainer hat on, I really would not want to live in that world.

Just pointing this out for completeness.

emmatown added a commit to keystonejs/keystone that referenced this pull request Mar 1, 2019
I'm working on code splitting things with suspense and things are breaking because of the findDOMNode calls in react-node-resolver

Slightly related to this: @jossmac, i think you'll like this, reactjs/rfcs#97
emmatown added a commit to keystonejs/keystone that referenced this pull request Mar 1, 2019
* Use render prop for target in withModalHandlers

I'm working on code splitting things with suspense and things are breaking because of the findDOMNode calls in react-node-resolver

Slightly related to this: @jossmac, i think you'll like this, reactjs/rfcs#97

* Update types

* Fix linting error

* Fix a thing

* Fix thing
emmatown added a commit to keystonejs/keystone that referenced this pull request Mar 25, 2019
* Add lazy loading stuff to field-views-loader

* More stuff

* Use render prop for target in withModalHandlers

I'm working on code splitting things with suspense and things are breaking because of the findDOMNode calls in react-node-resolver

Slightly related to this: @jossmac, i think you'll like this, reactjs/rfcs#97

* Update types

* Fix linting error

* Fix a thing

* Fix thing

* Works in some cases

* Fix a thing

* Add a Suspense component around page content

* Update CreateItemModal in Relationship field type

* Update UpdateManyItemsModal

* Everything is _mostly_ working now

* Fix a thing

* Fix some things

* forwardRef Pill

* Fix things

* Fix a thing

* Add changeset

* Add changeset

* Add changeset for @arch-ui/layout changes

* Add `compression` and change webpack config stuff so it works how it will work when it's on npm as a test

* Change webpack stuff back

* Try a thing to fix the access control tests

* Only do gzipping in prod
@intellild
Copy link

What is the status of this RFC, I am struggling with API designs.

@techird
Copy link

techird commented Jul 9, 2019

I hope this feature can be implemented sooner. This is the only reason for me that cant not get rid of class components.

@silverwind
Copy link

silverwind commented Jul 21, 2019

Isn't returning a Array of children quite a departure on how the current DOM refs (which point to a single DOM node) and findDOMNode(this) (which returns the first non-emtpy child) work?

I'm happy to have it return a Array of children, just a bit surprised by the fact that it does so. Strictly speaking, I guess it should be called refs, thought it's probably not worth the confusion that would bring.

@j-f1
Copy link

j-f1 commented Aug 2, 2019

@silverwind IMO it should still be called ref since it’s a reference to the fragment itself, which contains an array of DOM nodes as children.

@sebmarkbage
Copy link
Collaborator Author

Ok, so I've been researching use cases for findDOMNode or something like Fragment refs - and I don't think this is the right direction.

Taking a deeper look was originally spawned from seeing that fragment refs ends up being quite confusing when it comes to hidden offscreen content (such as prerendered, display locked or hidden due to a suspense boundary). You may end up getting more nodes than you'd expect. More over, it is too tempting to just assume or assert a single child node while not considering multi-node implementations like suspense boundaries.

Encapsulation

The fundamental benefits we get from React's component model is the constraints it enforces so that you can trust how other components behave. E.g. when their life-cycles fire, that they get there input from props/context etc. A parent can know something about what its children does, but a child shouldn't need to know about its parents. It's a critical property that you can refactor children without also refactoring all its uses. That empowers reuse. A key feature of that is being able to change the implementation details by using multiple DOM nodes or wrapping the root component in a Suspense boundary that renders two nodes, or rendering a fragment with one in-place DOM node and one portal. It's also about knowing which properties a parent is allowed to override by accepting explicit props or context to manage those overrides.

Neither findDOMNode nor Fragment refs preserve that quality. Since they give full access to the DOM node. The net effect is that components have to resort to defensive programming. E.g. adding an early wrapper just in case someone drills into the component. It also adds a risk that discourages refactoring such components.

forwardRef

forwardRef doesn't have this problem, as much, since it's an explicit reference that you opt-in to provide a public API that includes a particular DOM node. It also allows the ref to be redirected to a specific place so it can change position during refactors. You can also use named ref props to allow multiple refs.

Unfortunately it also encourages all these components to expose their entire DOM node. It's a reasonable escape hatch but not ideal. Ultimately I think we'll want to move towards not needing this as much. For example, if we had more like first-class focus management or something "scopes" based way of setting focus then components wouldn't need to expose a directly focusable thing.

For now it's a decent workaround though.

Use Cases

Use cases for findDOMNode tends to fall into roughly these categories:

  • Adding additional properties or styles to the child.
  • Adding additional event listeners to the child.
  • Setting focus on the child.
  • Various use cases related to the layout rectangle of a component, such as visibility tracking and scrolling into view.

Adding additiona properties or styles to a child is generally considered bad and not expected since it may collide with the styles already defined in a component. There's generally good intuition about this but it sometimes breaks down. E.g. is it ok to add an outline to a child component's styles when it's focused? What if there's two children in a fragment? Typically when this happens, it's a use case that could be better solved by a wrapper DOM element that takes on the size of the children.

In fact, all these cases can be solved by adding a wrapper DOM node. I'd argue that any use case that couldn't be solved by a wrapper DOM node is not legit since it risks conflicting with the child.

Even for things like event listeners, that can have two at once, this causes problems with unclear invocation order. It's not obvious when an event listener will get attached. E.g. normally a parent's life-cycle fire later so it probably attaches an event listener that is invoked after the child. However, if you call useEffect to attach and remove event listeners, then changes to the deps in the child may cause it to readd a new listener that is now invoked after the parent. You also have to be careful to also call stopImmediatePropagation at the right places. It's just much simpler if the event bubbling model is always related to parent and child.

Therefore, I argue that the default solution to all these problems should be to add a wrapper DOM node.

Why Not Wrapper DOM Elements?

In practice, it can be difficult to add a wrapper Element in the DOM for a number of reasons:

  • There might be perf considerations with having too many DOM nodes in the tree.
  • It can mess with CSS child selectors.
  • It can mess with layout in certain cases such as changing the flex environment.
  • The accessibility tree needs to be considered and aria used properly.
  • If the component is rendered between two special tags such as <tbody /> and <tr /> it can break server rendering because the HTML parser in browsers doesn't allow arbitrary tags to be inserted in this context.

The performance considerations for DOM nodes are real, but keep in mind that we're talking about a small linear increase. It's not exponential. In general when DOM nodes are too many it's because either windowing (virtualization) is not used, or you have too many complex CSS selectors. I'd also expect that the overall effect would be fewer DOM nodes or the same after a while given that reusable components now don't have to use as much defensive coding.

Regarding CSS child selectors, if abused in this way, they have the same encapsulation problems as findDOMNode. I'd argue that they shouldn't be used at all in components system and if used they should be restricted to not drill through the encapsulation. Even outside React, this is what Shadow DOM is for. If this remains a problem, maybe we should consider rendering components into Shadow DOM so that these abuses are not possible.

Defining layout of a wrapper can be tricky. Luckily display: contents is supposed to solve this. It does have some issues with a11y atm skipping over and into them but for the purpose that these are used I believe this can be sufficient. Similarly, the aria tree can be messed with things like grid-row/cell similar to HTML, but I hear "presentation" can be used for these intermediates.

HTML parsing and SSR is a problem that doesn't have an obvious solution in user space right now. The typical solution is to provide an optional tag name that can be provided in the cases this component is used in unusual contexts like a table.

Alternative 1: Reified Fragment Node

I think the ideal would be that the browser adds a first-class fragment tag that solves these issues by default. However, until then it's possible that we could build a polyfill in React. One possible idea is that when you render <fragment> we would create a real DOM element and automatically assign the right styles and a11y rules to it based on its context as to make it transparent.

To solve the HTML parsing problem, React could omit the fragment tags during HTML rendering in contexts where it's not allowed. Then during hydration it could add them back in. This works because when you imperatively append content to the DOM, invalid tags are allowed to exist in the tree.

Alternative 2: Virtual Fragment Node

Some things, like rendering an additional outline around a component, will need a real reified element. However many use cases are not adding additional behavior to the DOM. They're just observing something about the DOM. These typically fall into two categories:

  • Events
  • Bounding Rectangle

Events are pretty easy to do in React since we already have a virtual event system. We can add additional levels and bubble through a fragment just as if it was really in the DOM without reifing it.

<Fragment onClick={e => e.stopPropagation()}>...</Fragment>

The only thing is that, unlike a reified fragment, this wouldn't capture events on text nodes. You'd have to add an explicit wrapper for that.

Similarly, we could measure the client rectangle anytime the children resizes:

<Fragment onResize={boundingRect => ...}>...</Fragment>

When I say "bounding rect" I really mean some abstraction over the whole stacking context. So that you can figure out if it's obscured by other boxes. It might also need to be multiple rectangles when they're not adjacent such as the rectangle of a portal.

The use cases that require the bounding box of the child includes:

  • Determining if a component is visible or scrolled offscreen or obscured.
  • Scrolling a component into view by ensuring that scrolling includes the bounds of the component.
  • Positioning a tooltip next to or near the bounds of the component.
  • Clicking or setting focus in the middle of the component (for testing).

The bounding box is special because it is a primitive contract of a component. Every component ultimately takes up space in the layout of its parent (or zero if it's a portal/modal). This size can be trivially observed, e.g. adding a reified wrapper that sizes itself to its child and then measure that. This type of use case doesn't break encapsulation because you can completely change the implementation (including rendering a canvas even without DOM nodes) as long as it takes up layout somehow.

I think a lot of people have found this intuition given that these are the primary use cases where people use findDOMNode today.

So this is a very specific use case that we could add support for without giving full access to the DOM nodes.

Literally having an onResize callback with ResizeObserver on each child might be way overkill for some of these use cases though so the API should be designed to be implemented more efficiently for occasional measurements. Imperative on-demand measurements are not great since they observe where we currently are and not where we're going (concurrently). So ideally it should only expose sizes during commit (synchronized) life-times.

@sebmarkbage
Copy link
Collaborator Author

sebmarkbage commented Jan 27, 2020

I'm going to close this RFC out since I'm pretty convinced that this particular implementation is not a good idea.

It doesn't seem super critical as a way to move off findDOMNode. We've found that it is possible to migrate off it with some inconvenience and that's also what we've heard from the community.

I still think one or two of the alternatives I mentioned above would be good additions to React as a further convenience that doesn't break encapsulation.

I encourage someone to write up an RFC for each of the alternatives.

@sag1v
Copy link

sag1v commented Jan 27, 2020

Thanks @sebmarkbage for the research and detailed explanation. Virtual fragments sounds interesting.

@migueloller
Copy link

I would like to voice some support for Alternative 2 as it would solve our use-case of measuring the bounding rects of components in an arbitrary React tree of third-party components.

Solutions that introduce wrapping with reified elements are not compatible with what we need, but virtual nodes would work very similarly to <Fragment ref={...}> while keeping the reusability semantics mentioned.

@moecasts
Copy link

moecasts commented Jul 6, 2020

when i use run that code, it is warning that:

Warning: Invalid attribute `ref` supplied to `React.Fragment`.

How can I make it run successfully?

@juzhiyuan
Copy link

when i use run that code, it is warning that:

Warning: Invalid attribute `ref` supplied to `React.Fragment`.

How can I make it run successfully?

This RFC just closed

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.

None yet