[react] Revert "Allow React context in ReactNode" #69347
Conversation
|
@eps1lon 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": 69347,
"author": "eps1lon",
"headCommitOid": "7c3e9a7f2660883dbc40253beba8e58d473974e9",
"mergeBaseOid": "44c0d652bf5a29b126d891fb6bca3e5b80cebddb",
"lastPushDate": "2024-04-13T15:41:01.000Z",
"lastActivityDate": "2024-04-13T15:41:01.000Z",
"hasMergeConflict": false,
"isFirstContribution": false,
"tooManyFiles": false,
"hugeChange": false,
"popularityLevel": "Critical",
"pkgInfo": [
{
"name": "react",
"kind": "edit",
"files": [
{
"path": "types/react/canary.d.ts",
"kind": "definition"
},
{
"path": "types/react/test/canary.tsx",
"kind": "test"
},
{
"path": "types/react/test/index.ts",
"kind": "test"
},
{
"path": "types/react/ts5.0/canary.d.ts",
"kind": "definition"
},
{
"path": "types/react/ts5.0/test/canary.tsx",
"kind": "test"
},
{
"path": "types/react/ts5.0/test/index.ts",
"kind": "test"
}
],
"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",
"mattpocock"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Critical"
}
],
"reviews": [],
"mainBotCommentID": 2053682907,
"ciResult": "pass"
} |
|
🔔 @johnnyreilly @bbenezech @pzavolinsky @ericanderson @DovydasNavickas @theruther4d @guilhermehubner @ferdaber @jrakotoharisoa @pascaloliv @Hotell @franklixuefei @Jessidhia @saranshkataria @lukyth @zieka @dancerphil @dimitropoulos @disjukr @vhfmag @hellatan @priyanshurav @Semigradsky @mattpocock — please review this PR in the next few days. Be sure to explicitly select |
|
@eps1lon 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. |
Breaks render props when combined with "Switch <Context> to mean <Context.Provider>". We always hacked <Context.Provider> to work by treating it as a function. However, once we allow Context.Provider as a valid ReactNode, unions of ReactNode and functions (e.g. render props) can no longer refine the value via `typeof function` nor does contextual typing work (see added tests). We probably need to add correct types for Context and then adjust `JSX.ElementType` accordingly. Though I fear TS will no longer be able to infer props.
481b4ca
to
7c3e9a7
Compare
Reverts #69304
React context as ReactNode breaks render props when combined with "Switch to mean <Context.Provider>". We always hacked <Context.Provider> to work by treating it as a function. However, once we allow Context.Provider as a valid ReactNode, unions of ReactNode and functions (e.g. render props) can no longer refine the value via
typeof functionnor does contextual typing work (see added tests).We probably need to add correct types for Context and then adjust
JSX.ElementTypeaccordingly. Though I fear TS will no longer be able to infer props.React Context as a ReactNode is more for parity with usable values anyway. There's no observable difference between
<div>{React.createContext("value")}</div>and<div>{React.use(React.createContext("value"))}</div>so I don't think it's worth shipping it in types in this state.