Dark Mode

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[@types/react] add VoidFunctionComponent type which does not accept "children"#46643

Merged
typescript-bot merged 1 commit intoDefinitelyTyped:masterfrom
awmottaz:master
Aug 26, 2020
Merged

[@types/react] add VoidFunctionComponent type which does not accept "children"#46643
typescript-bot merged 1 commit intoDefinitelyTyped:masterfrom
awmottaz:master

Conversation

Copy link
Contributor

awmottaz commented Aug 9, 2020

Adds a type VoidFunctionComponent with VFC alias that is the same as
the FunctionComponent (FC) type, except that it does not accept a
children prop.

cf. #40993

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Add or edit tests to reflect the change. (Run with npm test.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).
  • Provide a URL to documentation or source code which provides context for the suggested changes: [@types/react] PropsWithChildren is incorrect or misleading. #40993 (comment)
  • If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.
  • Include tests for your changes
  • If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }. If for reason the any rule need to be disabled, disable it for that line using // tslint:disable-next-line [ruleName] and not for whole package so that the need for disabling can be reviewed.

dancerphil, k-yle, wongmjane, kachkaev, martin-eq, and agatsumayuki reacted with thumbs up emoji
awmottaz requested review from Jessidhia and johnnyreilly as code owners August 9, 2020 21:09
typescript-bot added the Critical package label Aug 9, 2020
Copy link
Contributor

typescript-bot commented Aug 9, 2020 *
edited
Loading

@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

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.


Diagnostic Information: What the bot saw about this PR
{
"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
}

Copy link
Contributor

typescript-bot commented Aug 9, 2020

typescript-bot added the The CI failed When GH Actions fails label Aug 9, 2020
Copy link
Contributor

typescript-bot commented Aug 9, 2020

@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!

eps1lon requested changes Aug 9, 2020
Copy link
Collaborator

eps1lon left a comment

Choose a reason for hiding this comment

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

Big fan of the idea. What do you consider void about this component?

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.

};

const VoidFunctionComponent3: React.VoidFunctionComponent =
// undesired: Rejects implicit usage of props.children
Copy link
Collaborator

eps1lon Aug 9, 2020

Choose a reason for hiding this comment

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

Since this component isn't supposed to accept children wouldn't the error be desired?

Copy link
Contributor Author

awmottaz Aug 9, 2020

Choose a reason for hiding this comment

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

Ah yes, sorry about the confusing verbiage here

typescript-bot added the Revision needed This PR needs code changes before it can be merged. label Aug 9, 2020
Copy link
Contributor

typescript-bot commented Aug 9, 2020

@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!

Copy link
Contributor Author

awmottaz commented Aug 9, 2020

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

eps1lon and mvasin reacted with thumbs up emoji

Copy link
Contributor Author

awmottaz commented Aug 9, 2020

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.

typescript-bot removed Revision needed This PR needs code changes before it can be merged. The CI failed When GH Actions fails labels Aug 9, 2020
Copy link
Collaborator

eps1lon commented Aug 9, 2020

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.

Copy link
Contributor Author

awmottaz commented Aug 9, 2020

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.

typescript-bot added the The CI failed When GH Actions fails label Aug 9, 2020
Copy link
Contributor

typescript-bot commented Aug 9, 2020

@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!

Copy link
Collaborator

eps1lon commented Aug 9, 2020

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.

Copy link
Contributor

typescript-bot commented Aug 10, 2020

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?

Comparison details
master #46643 diff
Batch compilation
Memory usage (MiB) 130.6 128.4 -1.7%
Type count 35619 37795 +6%
Assignability cache size 8532 8607 +1%
Language service
Samples taken 2683 2514 -6%
Identifiers in tests 2851 2928 +3%
getCompletionsAtPosition
Mean duration (ms) 284.6 293.7 +3.2%
Mean CV 7.7% 8.0%
Worst duration (ms) 1058.0 1114.4 +5.3%
Worst identifier x x
getQuickInfoAtPosition
Mean duration (ms) 280.6 290.1 +3.4%
Mean CV 7.8% 8.1% +3.6%
Worst duration (ms) 918.2 884.3 -3.7%
Worst identifier props ref

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

typescript-bot added the Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance. label Aug 10, 2020
eps1lon requested changes Aug 10, 2020
Copy link
Collaborator

eps1lon left a comment

Choose a reason for hiding this comment

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

Agree with the change and the naming makes sense to me now. React also uses that terminology:

input is a void element tag and must neither have children nor use dangerouslySetInnerHTML.

We need to figure out what's wrong with CI though.

*/
type ReactType

= ElementType

;

type ComponentType

= ComponentClass

| FunctionComponent

;

type ComponentType

= ComponentClass

| FunctionComponent

| VoidFunctionComponent

;

Copy link
Collaborator

eps1lon Aug 10, 2020

Choose a reason for hiding this comment

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

This might cause CI to fail? Or is master currently broken?

Copy link
Contributor Author

awmottaz Aug 10, 2020

Choose a reason for hiding this comment

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

master seems to be currently broken

Copy link
Contributor Author

awmottaz Aug 14, 2020

Choose a reason for hiding this comment

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

Thanks, this does seem to be the problem. I'm trying it out now with just using VoidFunctionComponent here. Does this look okay to you?

Suggested change
type ComponentType<P = {}> = ComponentClass<P> | FunctionComponent<P> | VoidFunctionComponent<P>;
type ComponentType<P = {}> = ComponentClass<P> | VoidFunctionComponent<P>;

Copy link
Contributor Author

awmottaz Aug 14, 2020

Choose a reason for hiding this comment

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

Ah, this is no good either. Going to try by reverting this line without VoidFunctionComponent.

typescript-bot added the Revision needed This PR needs code changes before it can be merged. label Aug 10, 2020
Copy link
Contributor

typescript-bot commented Aug 10, 2020

@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!

Copy link
Collaborator

eps1lon commented Aug 14, 2020

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

typescript-bot removed the Revision needed This PR needs code changes before it can be merged. label Aug 14, 2020
Copy link
Contributor

typescript-bot commented Aug 14, 2020

@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!

Copy link
Contributor Author

awmottaz commented Aug 14, 2020

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

Copy link
Contributor

ferdaber commented Aug 14, 2020 *
edited
Loading

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

typescript-bot added the Untested Change This PR does not touch tests label Aug 19, 2020
ferdaber approved these changes Aug 19, 2020
xRovern mentioned this pull request Aug 19, 2020
typescript-bot added the Self Merge This PR can now be self-merged by the PR author or an owner label Aug 26, 2020
Copy link
Contributor Author

awmottaz commented Aug 26, 2020

Ready to merge

typescript-bot merged commit 14d95eb into DefinitelyTyped:master Aug 26, 2020
Copy link
Contributor

typescript-bot commented Aug 26, 2020

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

chivesrs pushed a commit to chivesrs/DefinitelyTyped that referenced this pull request Sep 2, 2020
...nent type which does not accept "children" by @awmottaz

Adds a type `VoidFunctionComponent` with `VFC` alias that is the same as
the `FunctionComponent` (`FC`) type, except that it does not accept a
`children` prop.
danielrearden pushed a commit to danielrearden/DefinitelyTyped that referenced this pull request Sep 22, 2020
...nent type which does not accept "children" by @awmottaz

Adds a type `VoidFunctionComponent` with `VFC` alias that is the same as
the `FunctionComponent` (`FC`) type, except that it does not accept a
`children` prop.
Copy link
Contributor

ljharb commented Feb 2, 2021

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"?

gnoesiboe, jahglow, mvasin, and BasixKOR reacted with thumbs up emoji

Copy link

jahglow commented Mar 17, 2021

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

Copy link
Contributor

ljharb commented Mar 17, 2021

cc @eps1lon re #46643 (comment)

Copy link
Collaborator

eps1lon commented Mar 17, 2021

We settled on "void component" since that is established terminology in React (e.g. foo 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.

Copy link
Contributor

ljharb commented Mar 18, 2021 *
edited
Loading

@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.

mvasin and acton-bell reacted with thumbs up emoji

Copy link
Collaborator

eps1lon commented Mar 18, 2021

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. void element is different from a Input void function component? Neither accept children.

Copy link
Contributor

ljharb commented Mar 18, 2021 *
edited
Loading

Incorrect:

function RendersChildrenButDoesNotAcceptChildren() {
return <div>childrendiv>;
}
function AcceptsChildrenButDoesNotRenderChildren({ children }) {
return <input />;
}
acton-bell reacted with thumbs up emoji

Copy link
Collaborator

eps1lon commented Mar 18, 2021

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.

Copy link
Contributor

ljharb commented Mar 18, 2021

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 childrendiv>
}

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

mvasin and acton-bell reacted with thumbs up emoji ionut-botizan reacted with thumbs down emoji

Copy link

esebastian commented Mar 19, 2021

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: vs Some children.

Copy link
Contributor

ljharb commented Mar 19, 2021

@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. passes no children, Some children and 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.

mvasin, alex-kinokon, and acton-bell reacted with thumbs up emoji

Copy link
Contributor

mvasin commented Mar 22, 2021 *
edited
Loading

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).

smac89 reacted with thumbs up emoji

Copy link
Contributor

flying-sheep commented Dec 17, 2021 *
edited
Loading

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

eps1lon eps1lon approved these changes

Jessidhia Awaiting requested review from Jessidhia Jessidhia is a code owner

johnnyreilly Awaiting requested review from johnnyreilly johnnyreilly is a code owner

+1 more reviewer

ferdaber ferdaber approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Critical package Owner Approved A listed owner of this package signed off on the pull request. Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance. Self Merge This PR can now be self-merged by the PR author or an owner

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants