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

RFC: "root element" #15138

Closed
2 tasks
eps1lon opened this issue Apr 1, 2019 · 9 comments · Fixed by #15337
Closed
2 tasks

RFC: "root element" #15138

eps1lon opened this issue Apr 1, 2019 · 9 comments · Fixed by #15337
Labels
discussion docs Improvements or additions to the documentation

Comments

@eps1lon
Copy link
Member

eps1lon commented Apr 1, 2019

Our docs currently use the term "root element" to describe where excess props are spread.

This term is currently not explicitly defined and has room for ambiguity. The goal of this issue is to find a definition and apply that to every core component to reduce surprising API behavior.

Definition

root = outermost

If we take the rendered output from a component one could argue that the "root" is the top element e.g.

function Component() {
  return (
    <RootElement>
      <NotARootElement>
        <NotARootElementEither />
      </NotARootElement>
    </RootElement>
  );
}

Drawbacks

This would not apply to many components currently (Popper, TableHead etc.). In general this definition wouldn't be very useful since every component introduced that wouldn't even change the behavior would be a breaking change (e.g. Context.Provider, StrictMode, Fragment, EventListener).

root = outermost host

function Component() {
  return (
    <NotARootElement>
      <div data-is-root={true}>
        <NotARootElementEither />
      </div>
    </NotARootElement>
  );
}

Most of the components would currently be covered by defining the "root element" as the outermost element that renders a host element.

Drawbacks

It is not obvious by looking at the implementation what the root is. Maintainers might know this but without any context we don't know what components render host elements. If we look at the example it is not obvious if NotARootElement is rendering a host element or not (see our transition components).

This definition would break when portaling or when we add wrapper containers for styling purposes.

It is also not very useful for testing. Let's say we define that the ListItem is the root element of a MenuItem. We want to test that props for the ListItem are actually spread to it but the definition provides no selector to find the ListItem and check if every prop is there. What we would find is an li host element and that shouldn't have any props for the ListItem at that point.

root = defined per component

The root element would be what we define via @inheritComponent. The definition would add the following constraints to the component:

  1. The root element has the root class
  2. Excess props are spread to this element
  3. The root element has the type of inheritComponent
  4. The ref is forwarded to the root element (unclear if this adds value)
  5. The root element renders the outermost host element (undecided

remarks

about 4.

For measuring it would make sense to apply the ref to the outermost host. Can be mitigated by exposing an additional ref prop that is applied to the outermost host

about "host element"

It might be ok to use it in the following context: "Excess props are spread to the root element". However, saying that "Paper is the root element" is technically not correct. "Paper would be the root component" (or element type; but that's an uncommon term). "<Paper /> would be an actual root element". This is just nitpicking but while we're at it we might as well do it right.

/cc @mui-org/core-contributors

@eps1lon eps1lon added docs Improvements or additions to the documentation core discussion labels Apr 1, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 1, 2019

Thanks for opening this issue.

Our docs currently use the term "root element" to describe where excess props are spread.

Given I'm responsible for most of the current behavior, I can put light on the design decision. It's a chicken-egg problem. The props are spread on the "root element", not the other way around. My definition was: the root element is the first outermost element that renders a host element. It should cover 99% of the current implementation. The only exception I'm aware of is <ListItem button />. Also, we are considering a new exception with InputBase. It's a complex problem. Why was it designed this way?
Following your surprising API principle, we have to merge two different domains: the React and HTML word. We should do our best to minimize the difference between these two domains. I believe most people debug React with the HTML dev tools, not the React one. So, I think that we should optimize for HTML, it's more intuitive.

@eps1lon
Copy link
Member Author

eps1lon commented Apr 1, 2019

My problem is that this is only useful if the root element is a host element. For composite components (e.g. Paper, ListItem) it doesn't make much sense. Especially when testing the notion of "top host component" doesn't provide any selector for checking if the "root element" is of type Paper.

Issues also arise with the common root class which also uses the term "root element". So there is clearly some behavior we should test ("root element".hasClass(classes.root)). Maybe I should've emphasized this more. It's not only about props spread but also about the CSS API which also uses the term "root element". In conjunction with a predictable customization API it's probably a good idea to expose a common root class API.

In the end it's perfectly fine to add a constraint that the root element should render the first host (first by breadth-first search; it's important for SwipeableDrawer). Strictly from a React selector POV wrapper.find(RootComponentConstructor).filter(.${classes.root}) is the only unambiguous selector as far as I can tell.

What's really important is that we can't just say we optimize for HTML. Not if we document that excess props are spread to composite components.

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 1, 2019

For composite components (e.g. Paper, ListItem) it doesn't make much sense.

What's the issue with the Paper? I miss one part of the 🧩.

Especially when testing the notion of "top host component" doesn't provide any selector for checking if the "root element" is of type Paper.

I think that we should optimize for usage, not for the ease of writing tests in the repository. But if the tests are hard to right, it could be a sign of a poor API.

So there is clearly some behavior we should test ("root element".hasClass(classes.root)).

You are right, it's exactly what the definition I have given previously implies. The CSS API works in conjunction with the HTML API. I think that the ref, the classes.root string and the rest props should end up on the root element's host (using this definition). It's what I mean by "optimizing" for HTML over React.

first by breadth-first search

Yes, it's important for the SwipeableDrawer or the Tooltip. Thanks for the precision 👌.

Strictly from a React selector POV wrapper.find(RootComponentConstructor).filter(.${classes.root}) is the only unambiguous selector as far as I can tell.

What do you think of?

const foo = randomStringValue();

const wrapper = mount(React.cloneElement(element, { 'data-foo': foo }));

assert.strictEqual(findOutermostIntrinsic(wrapper).hasClass(classes.root), true);
assert.strictEqual(findOutermostIntrinsic(wrapper).props()['data-foo'], foo);
testRef(element, mount, current => assert.strictEqual(Array.from(current.classList).indexOf(classes.root) !== -1, true));

What's really important is that we can't just say we optimize for HTML. Not if we document that excess props are spread to composite components.

It's recursive. We could document the full spread chain down to the root element's host. It would save people clicks.

@eps1lon
Copy link
Member Author

eps1lon commented Apr 1, 2019

I think that we should optimize for usage, not for the ease of writing tests in the repository. But if the tests are hard to right, it could be a sign of a poor API.

This is what I'm getting at. I agree that tests shouldn't be the priority. I'm more focusing on the aspect that in a component tree it's not obvious what the root element is i.e. there is no apparent selector. Apparent meaning that I don't know it by looking at the render implementation of a component. Only by inspecting the implementation of the children I know what the root element is.

What's the issue with the Paper? I miss one part of the

Not strictly Paper but any composite. If we say the "root element is a Paper" then the "outermost host element" doesn't make much sense because a component prop like elevation isn't spread to the host element but the Paper element.

By your definition (root element = outermost host element) Paper would not be the root element of AppBar but header.

I don't think we disagree what the "root element" is. My definition would just leave no room for interpretation when @inheritComponent is a composite component. And it adds some more constraints that make class order predictable across components and adds a common customization API (every component has a root class that can be accessed via overrides).

It's recursive. We could document the full spread chain down to the root element's host. It would save people clicks.

This is something I definitely want to solve. I find myself often annoyed by having to open multiple tabs to check what props are allowed.

@oliviertassinari
Copy link
Member

Apparent meaning that I don't know it by looking at the render implementation of a component. Only by inspecting the implementation of the children I know what the root element is.

If the behavior is consistent, you don't need to look at anything. It's predictable :).

Not strictly Paper but any composite. If we say the "root element is a Paper" then the "outermost host element" doesn't make much sense because a component prop like elevation isn't spread to the host element but the Paper element.

I don't follow. It's not the "outermost host element", it's the outermost element that renders a host element.

By your definition (root element = outermost host element)

The root element is in the React domain, the outermost host element is in the HTML domain, we can't get an equality. What's your definition?

I find myself often annoyed by having to open multiple tabs to check what props are allowed.

Same for me :).

@eps1lon
Copy link
Member Author

eps1lon commented Apr 2, 2019

If the behavior is consistent, you don't need to look at anything. It's predictable :).

How can you infer from <SomeUnknownComponentYouDontKnowAnythingAbout className="root"> that it renders a host element?

I don't follow. It's not the "outermost host element", it's the outermost element that renders a host element.

Well yes that is the precise issue. We don't know if a component renders a host element. This information is only available upon inspection of the component.

The root element is in the React domain, the outermost host element is in the HTML domain, we can't get an equality. What's your definition?

Host element is a term in the React domain. What is considered a valid host element depends on the renderer.

@oliviertassinari
Copy link
Member

How can you infer from that it renders a host element?

You can't, welcome to React 🙃. Seriously, documentation can help our users here 🙂.

@eps1lon
Copy link
Member Author

eps1lon commented Apr 3, 2019

I think "root component = component that renders the outermost host component (where outermost is the first hit when doing BFS)" works (as in it's testable by computers and understandable by humans).

We can add a section to the API design section explaining this term.

@oliviertassinari
Copy link
Member

💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants