[react] Mark error from componentDidCatch to be unknown#69434
[react] Mark error from componentDidCatch to be unknown#69434AbhiPrasad wants to merge 1 commit intoDefinitelyTyped:masterfrom
unknown#69434Conversation
Given anything can be thrown in JavaScript, it is more accurate to treat the `error` argument` that is passed into `componentDidCatch` as `unknown`. That allows users to explicitly handle it via `instanceof Error`, or understand if something in their components is throwing a non-error (object, string or primitive).
|
@AbhiPrasad Thank you for submitting this PR! This is a live comment which I will keep updated. 1 package in this PRCode ReviewsBecause this is a widely-used package, a DT maintainer will need to review it before it can be merged. You can test the changes of this PR in the Playground. Status
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": 69434,
"author": "AbhiPrasad",
"headCommitOid": "a0c758e11b3d576e45ac8ca8430b0ced50656339",
"mergeBaseOid": "94976793273991f9c3e3228fe971ec65a60cb2df",
"lastPushDate": "2024-04-24T17:29:27.000Z",
"lastActivityDate": "2024-04-24T18:59:08.000Z",
"hasMergeConflict": false,
"isFirstContribution": false,
"tooManyFiles": false,
"hugeChange": false,
"popularityLevel": "Critical",
"pkgInfo": [
{
"name": "react",
"kind": "edit",
"files": [
{
"path": "types/react/index.d.ts",
"kind": "definition"
},
{
"path": "types/react/v16/index.d.ts",
"kind": "definition"
},
{
"path": "types/react/v17/index.d.ts",
"kind": "definition"
}
],
"owners": [
"johnnyreilly",
"bbenezech",
"pzavolinsky",
"ericanderson",
"DovydasNavickas",
"theruther4d",
"guilhermehubner",
"ferdaber",
"jrakotoharisoa",
"pascaloliv",
"hotell",
"franklixuefei",
"Jessidhia",
"saranshkataria",
"lukyth",
"eps1lon",
"zieka",
"dancerphil",
"dimitropoulos",
"disjukr",
"vhfmag",
"hellatan",
"priyanshurav",
"Semigradsky"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Critical"
}
],
"reviews": [
{
"type": "changereq",
"reviewer": "eps1lon",
"date": "2024-04-24T18:35:26.000Z"
}
],
"mainBotCommentID": 2075475086,
"ciResult": "fail",
"ciUrl": "https://github.com/DefinitelyTyped/DefinitelyTyped/commit/a0c758e11b3d576e45ac8ca8430b0ced50656339/checks?check_suite_id=23112076139"
} |
|
🔔 @johnnyreilly @bbenezech @pzavolinsky @ericanderson @DovydasNavickas @theruther4d @guilhermehubner @ferdaber @jrakotoharisoa @pascaloliv @Hotell @franklixuefei @Jessidhia @saranshkataria @lukyth @eps1lon @zieka @dancerphil @dimitropoulos @disjukr @vhfmag @hellatan @priyanshurav @Semigradsky — please review this PR in the next few days. Be sure to explicitly select |
|
@AbhiPrasad The CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review. |
|
I get the following error because other libraries also use the same assumption with https://github.com/DefinitelyTyped/DefinitelyTyped/actions/runs/8820759030/job/24215116748?pr=69434 Should I make changes everywhere else before moving forward with the change here? |
There was a problem hiding this comment.
Definitely a good change we should make.
We can do this for React 20 types but not older versions. 19 is already finished and I don't want to add any last minute breaking changes that risk delaying types for 19.
We generally don't make something unknown outside of major release. Especially since the types existed for this for so long.
|
@AbhiPrasad 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! |
|
Sounds like a plan, thank you @eps1lon should I close this PR and re-visit when React 20 is being actively worked on? Or is there a way to add these types in now? |
|
We'll just cherry-pick this when we start working on it. Can be closed in the meantime. |
Fixes #11728 As reporting in the above issue, it is not typesafe to type the error thrown by react components as `Error`, it can actually be any JS object/primitive that was thrown. This means we have to type everything as `unknown`. This change only will happen for `8.x` because it's a breaking change. Related: - https://react.dev/reference/react/Component#componentdidcatch - DefinitelyTyped/DefinitelyTyped#69434 --------- Co-authored-by: Francesco Novy <francesco.novy@sentry.io>
Given anything can be thrown in JavaScript, it is more accurate to treat the
errorargument that is passed intocomponentDidCatchasunknown. That allows users to explicitly handle it viainstanceof Error, or understand if something in their components is throwing a non-error (object, string or primitive).Please fill in this template.
pnpm test <package to test>.Select one of these and delete the others:
If changing an existing definition:
componentDidCatchtakes anerrorthat comes fromerrorInfo.value.https://github.com/facebook/react/blob/cb151849e13f46ec64570519cb93d5939fb60cab/packages/react-reconciler/src/ReactFiberThrow.js#L150-L154
which is typed as
CapturedValue<mixed>https://github.com/facebook/react/blob/cb151849e13f46ec64570519cb93d5939fb60cab/packages/react-reconciler/src/ReactFiberThrow.js#L118
which is a generic
https://github.com/facebook/react/blob/cb151849e13f46ec64570519cb93d5939fb60cab/packages/react-reconciler/src/ReactCapturedValue.js#L16-L20
which means that
errorInfo.valueis typed asmixedwith flow, meaning it should beunknownhere.ref: getsentry/sentry-javascript#11728