Skip to content

fix(react): React.forwardRef return type should have 'children' props#52604

Closed
csu-feizao wants to merge 1 commit intoDefinitelyTyped:masterfrom
csu-feizao:fix-react-forward-ref-type
Closed

fix(react): React.forwardRef return type should have 'children' props#52604
csu-feizao wants to merge 1 commit intoDefinitelyTyped:masterfrom
csu-feizao:fix-react-forward-ref-type

Conversation

@csu-feizao
Copy link
Copy Markdown

Please fill in this template.

Select one of these and delete the others:

If changing an existing definition:

  • Provide a URL to documentation or source code which provides context for the suggested changes: <>
  • 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.

@typescript-bot
Copy link
Copy Markdown
Contributor

typescript-bot commented Apr 27, 2021

@csu-feizao 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 this is a widely-used package, a DT maintainer will need to review it before it can be merged.

Status

  • ✅ No merge conflicts
  • ✅ Continuous integration tests have passed
  • 🕐 Most recent commit is approved by a DT maintainer

Once every item on this list is checked, I'll ask you for permission to merge and publish the changes.


Diagnostic Information: What the bot saw about this PR
{
  "type": "info",
  "now": "-",
  "pr_number": 52604,
  "author": "csu-feizao",
  "headCommitOid": "3b44184f73c27e3dd8285561f18d580802d92959",
  "lastPushDate": "2021-04-27T03:28:51.000Z",
  "lastActivityDate": "2021-04-27T08:11:23.000Z",
  "maintainerBlessed": false,
  "hasMergeConflict": false,
  "isFirstContribution": true,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "react",
      "kind": "edit",
      "files": [
        {
          "path": "types/react/v16/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/react/v16/test/tsx.tsx",
          "kind": "test"
        }
      ],
      "owners": [
        "johnnyreilly",
        "bbenezech",
        "pzavolinsky",
        "digiguru",
        "ericanderson",
        "DovydasNavickas",
        "theruther4d",
        "guilhermehubner",
        "ferdaber",
        "jrakotoharisoa",
        "pascaloliv",
        "hotell",
        "franklixuefei",
        "Jessidhia",
        "saranshkataria",
        "lukyth",
        "eps1lon",
        "zieka",
        "dancerphil",
        "dimitropoulos",
        "disjukr",
        "vhfmag",
        "hellatan"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    }
  ],
  "reviews": [
    {
      "type": "changereq",
      "reviewer": "eps1lon",
      "date": "2021-04-27T08:11:23.000Z"
    }
  ],
  "ciResult": "pass"
}

@typescript-bot
Copy link
Copy Markdown
Contributor

@typescript-bot
Copy link
Copy Markdown
Contributor

👋 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 #52604 diff
Batch compilation
Memory usage (MiB) 121.7 122.5 +0.7%
Type count 35681 35681 0%
Assignability cache size 9988 9988 0%
Language service
Samples taken 2860 2860 0%
Identifiers in tests 2939 2939 0%
getCompletionsAtPosition
    Mean duration (ms) 264.2 266.4 +0.8%
    Mean CV 7.9% 7.8%
    Worst duration (ms) 1127.4 1067.4 -5.3%
    Worst identifier props ref
getQuickInfoAtPosition
    Mean duration (ms) 265.8 267.6 +0.7%
    Mean CV 9.4% 9.1%
    Worst duration (ms) 864.0 870.2 +0.7%
    Worst identifier ref ref

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

@typescript-bot typescript-bot added the Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance. label Apr 27, 2021
Copy link
Copy Markdown
Collaborator

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Duplicate of #48420:

We plan to remove implicit children in a future major React release since they are problematic: #33006

-- #48420 (review)

@typescript-bot typescript-bot added the Revision needed This PR needs code changes before it can be merged. label Apr 27, 2021
@typescript-bot
Copy link
Copy Markdown
Contributor

@csu-feizao 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. Thank you!

@csu-feizao
Copy link
Copy Markdown
Author

Duplicate of #48420:

We plan to remove implicit children in a future major React release since they are problematic: #33006

-- #48420 (review)

got, I'll close this PR.

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

Labels

Critical package Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance. Revision needed This PR needs code changes before it can be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants