@awmottaz Thank you for submitting this PR! I see this is your first time submitting to DefinitelyTyped 👋 — I'm the local bot who will help you through the process of getting things through.

This is a live comment which I will keep updated.

1 package in this PR

• react on npm, on unpkg

Code Reviews

Because you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it.

Status

• ✅ No merge conflicts
• ✅ Continuous integration tests have passed
• ✅ Most recent commit is approved by type definition owners or DT maintainers

All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes.

{
  "type": "info",
  "now": "-",
  "pr_number": 46643,
  "author": "awmottaz",
  "owners": [
    "johnnyreilly",
    "bbenezech",
    "pzavolinsky",
    "digiguru",
    "ericanderson",
    "DovydasNavickas",
    "theruther4d",
    "guilhermehubner",
    "ferdaber",
    "jrakotoharisoa",
    "pascaloliv",
    "hotell",
    "franklixuefei",
    "Jessidhia",
    "saranshkataria",
    "lukyth",
    "eps1lon",
    "zieka",
    "dancerphil",
    "dimitropoulos",
    "disjukr",
    "vhfmag",
    "hellatan"
  ],
  "dangerLevel": "ScopedAndTested",
  "headCommitAbbrOid": "9aa5631",
  "headCommitOid": "9aa5631278d887b0f5594e76041b7cc316564d5e",
  "mergeIsRequested": true,
  "stalenessInDays": 0,
  "lastPushDate": "2020-08-19T13:16:05.000Z",
  "lastCommentDate": "2020-08-26T19:31:40.000Z",
  "maintainerBlessed": true,
  "reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/46643/files",
  "hasMergeConflict": false,
  "authorIsOwner": false,
  "isFirstContribution": true,
  "popularityLevel": "Critical",
  "newPackages": [],
  "packages": [
    "react"
  ],
  "files": [
    {
      "path": "types/react/index.d.ts",
      "kind": "definition",
      "package": "react"
    },
    {
      "path": "types/react/test/tsx.tsx",
      "kind": "test",
      "package": "react"
    }
  ],
  "hasDismissedReview": false,
  "ciResult": "pass",
  "lastReviewDate": "2020-08-19T13:42:07.000Z",
  "reviewersWithStaleReviews": [],
  "approvalFlags": 2,
  "isChangesRequested": false
}

🔔 @johnnyreilly @bbenezech @pzavolinsky @digiguru @ericanderson @DovydasNavickas @theruther4d @guilhermehubner @ferdaber @jrakotoharisoa @pascaloliv @Hotell @franklixuefei @Jessidhia @saranshkataria @lukyth @eps1lon @zieka @dancerphil @dimitropoulos @disjukr @vhfmag @hellatan — please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

@awmottaz The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

@awmottaz One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. If you disagree with the reviewer's comments, you can "dismiss" the review using GitHub's review UI. Thank you!

What do you consider void about this component?

The naming is inspired by the analogy of "normal element" → FunctionComponent, so "void element" → VoidFunctionComponent
https://html.spec.whatwg.org/multipage/syntax.html#elements-2

I think most the types maintainers agree that this should be the default in new major release of @types/react so we might want to reflect that in the name.

Do you mean that the FunctionComponent type itself should be changed to what I have called VoidFunctionComponent?

I could use some help with the tests. It's not clear to me what is causing the failure, and I saw failures when I ran the tests before adding my changes.

Do you mean that the FunctionComponent type itself should be changed to what I have called VoidFunctionComponent?

Yes, we generally advise for that specific reason against using React.FunctionComponent. But removing implicit children would likely break a lot of things so we want to wait for React 17.

Yes, we generally advise for that specific reason against using React.FunctionComponent.

Just curious where that advice has been given? Maybe you can comment on #40993?

But removing implicit children would likely break a lot of things so we want to wait for React 17.

Yes, there is quite a lot that would probably break. For that reason, I think that a new type might be pragmatic and get something more useful to developers sooner and avoid the refactoring lift later. That change would force every FunctionComponent to define its own children prop and could be very painful for some projects. But I'm happy to defer to the maintainers of this package and would love to hear the counter-arguments in favor of waiting for v17 and modifying FunctionComponent instead of introducing a new type.

@awmottaz The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Just curious where that advice has been given?

Informal in https://react-typescript-cheatsheet.netlify.app/docs/basic/getting-started/function_components#docsNav, various conversations in this repo etc. Though we probably should add a warning to the JSDOC and recommend VoidFunctionComponent + explicit children.

👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings.

Let’s review the numbers, shall we?

It looks like nothing changed too much. I won’t post performance data again unless it gets worse.

@awmottaz One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. If you disagree with the reviewer's comments, you can "dismiss" the review using GitHub's review UI. Thank you!

@awmottaz Rebasing onto master should this PR go green now.

@awmottaz The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Confirmed that with latest master the tests pass. I'm looking into these new test failures.

@eps1lon would we be looking to deprecate VFC on the next major? Or just aliasing the FC type to VFC?

Ready to merge

I just published @types/react@16.9.48 to npm.

This is a really confusing choice of name. "void x" usually means "an x that returns nothing", and "accepting children" has no correlation to whether the component renders something or not.

Is it too late to perhaps provide a better name (leaving the old one as an alias as needed, for compat)? Perhaps, like HTML, an element that takes no children should be called "self-closing"?

Yeah, I wonder how this PR was merged with such a name, a disgrace to React Community commits a harakiri

cc @eps1lon re #46643 (comment)

We settled on "void component" since that is established terminology in React (e.g. <input>foo</input> warns about input is a void element tag) which is used because it is established terminology in HTML itself:

Void elements can't have any contents (since there's no end tag, no content can be put between the start tag and the end tag).

-- https://html.spec.whatwg.org/multipage/syntax.html#elements-2

So it does make sense to me to call them "VoidFunctionComponent" in the react domain. But I can see that this might clash with how we use "void" in javascript itself?

We don't intend to keep the name around anyway. VoidFunctionComponent will become FunctionComponent (probably once React 18 lands) i.e. typings will no longer declare implicit children.

Maybe I'm misinterpreting the HTML spec but if you agree that "void" does make sense with respect to HTML I'd rather stick with the name (to not create even more type names) since it's temporary anyway.

@eps1lon right but that's because it doesn't render its children. It still accepts them. This type is about props, not "what renders".

In other words, I do not agree this naming makes sense in any arena. "void elements" are ones that do not render children. Components can accept or not accept children, and that has zero guaranteed correlation to whether they render children or not.

Components can accept or not accept children, and that has zero guaranteed correlation to whether they render children or not.

If a component does not accept children it cannot render them. That is a guarantee. But I understand that components that do accept children do not guarantee that they render them e.g. <select />.

Could you clarify how a <input> void element is different from a Input void function component? Neither accept children.

Incorrect:

function RendersChildrenButDoesNotAcceptChildren() {
  return <div>children</div>;
}

function AcceptsChildrenButDoesNotRenderChildren({ children }) {
  return <input />;
}

Incorrect:

Which part is incorrect?

RendersChildrenButDoesNotAcceptChildren does not accept children and therefore doesn't render them. It does however render something. I didn't mean to imply that VoidFunctionComponents render nothing. Just like "void elements" don't render nothing.

It renders children because the element it produces has children. "renders children" does not mean "renders the children prop it receives".

function RendersChildrenAndAcceptsChildren({ children }) {
  return <div>different children</div>
}

Void elements render one thing with no children. For custom components, that is entirely decoupled from whether it accepts children as props or not.

I find it hard to follow your point of view @ljharb, because for me this is not about what the component is rendering (that's precisely what the component is abstracting), but about how it's being used: <MyComponent /> vs <MyComponent>Some children</MyComponent>.

@esebastian i totally agree, that's why i think "void element" is the wrong term - because that describes rendering behavior not interface behavior, and the type being used here describes the interface behavior. <MyComponent /> passes no children, <MyComponent>Some children</MyComponent> and <MyComponent children="some children" /> both do, and the type referenced in this PR has absolutely nothing to do with the rendering behavior of MyComponent - thus, it should have a distinct term from one that describes the rendering behavior.

Seems like this naming originates from the HTML spec:

Void elements only have a start tag; end tags must not be specified for void elements.

The quote above is about the input data type - one can't pass anything between the opening and closing tags to a "void" (in HTML spec terms) element.

And then the spec goes on with

Void elements can't have any contents (since there's no end tag, no content can be put between the start tag and the end tag).

This is great! I switched everything to VFC. Always nice to tighten types!

cross-posting my comment on the name from jsx-eslint/eslint-plugin-react#2913 (comment):

(sorry for the “horrible” … I simply think that giving it a more generic name implying it’s a supertype of “FC” would have prevented a lot of confusion)

I like that they’ll delete FC and rename VFC→FC in the next major release. That’s how FC should have been defined from the start.

If that’s the intention, VFC is truly a horrible name though. it should be named E(xplicit)FC then: “VFC” implies the upgrade path is:

• use FC for components taking children, specify child type in props interface only if it’s not ReactNode
• use VFC for components not taking children
• sort out the mess when upgrading to the next major typing version

I much prefer this upgrade path:

• use import { VFC as FC } from 'react' everywhere, explicitly state child types (or lack thereof) everywhere
• (optional) add this to your .eslintrc.yaml:

  no-restricted-imports:
  - error
  - paths:
    - name: react
      importNames: [FC]
      message: 'Use VFC instead and directly type children (e.g. `children?: ReactNode`)'

• delete VFC as (and eslint rule) in next major typing version