[react] Add JSX namespace to React namespace #64464
Conversation
eps1lon
force-pushed
the
feat/react/scoped-jsx-andarist
branch
from
0e1d93a to
9da1d85
Compare
eps1lon
force-pushed
the
feat/react/scoped-jsx-andarist
branch
2 times, most recently
from
b53ab5c to
6607992
Compare
eps1lon
changed the title
eps1lon
force-pushed
the
feat/react/scoped-jsx-andarist
branch
10 times, most recently
from
27c0b47 to
0181198
Compare
|
Inspecting the JavaScript source for this package found some properties that are not in the .d.ts files. react (unpkg)was missing the following properties:
|
|
@eps1lon Thank you for submitting this PR! This is a live comment which I will keep updated. 2 packages in this PRCode ReviewsThis PR can be merged once it's reviewed. 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. InactiveThis PR has been inactive for 7 days. Diagnostic Information: What the bot saw about this PR{
"type": "info",
"now": "-",
"pr_number": 64464,
"author": "eps1lon",
"headCommitOid": "3dea0576b786da0267a9a890926d86aa62aa435f",
"mergeBaseOid": "1e4da9b225c67b2b14ecbcaf90da63c0a193f5d2",
"lastPushDate": "2023-04-28T18:42:03.000Z",
"lastActivityDate": "2023-05-06T11:15:12.000Z",
"maintainerBlessed": "Waiting for Code Reviews",
"hasMergeConflict": false,
"isFirstContribution": false,
"tooManyFiles": false,
"hugeChange": false,
"popularityLevel": "Critical",
"pkgInfo": [
{
"name": "react-blessed",
"kind": "edit",
"files": [
{
"path": "types/react-blessed/index.d.ts",
"kind": "definition"
},
{
"path": "types/react-blessed/react-blessed-tests.tsx",
"kind": "test"
}
],
"owners": [
"guoshencheng",
"defnotrobbie"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Well-liked by everyone"
},
{
"name": "react",
"kind": "edit",
"files": [
{
"path": "types/react/index.d.ts",
"kind": "definition"
},
{
"path": "types/react/jsx-dev-runtime.d.ts",
"kind": "definition"
},
{
"path": "types/react/jsx-runtime.d.ts",
"kind": "definition"
},
{
"path": "types/react/test/tsx.tsx",
"kind": "test"
},
{
"path": "types/react/ts5.0/index.d.ts",
"kind": "definition"
},
{
"path": "types/react/ts5.0/jsx-dev-runtime.d.ts",
"kind": "definition"
},
{
"path": "types/react/ts5.0/jsx-runtime.d.ts",
"kind": "definition"
},
{
"path": "types/react/ts5.0/test/tsx.tsx",
"kind": "test"
},
{
"path": "types/react/v15/index.d.ts",
"kind": "definition"
},
{
"path": "types/react/v15/test/tsx.tsx",
"kind": "test"
},
{
"path": "types/react/v16/index.d.ts",
"kind": "definition"
},
{
"path": "types/react/v16/test/tsx.tsx",
"kind": "test"
},
{
"path": "types/react/v17/index.d.ts",
"kind": "definition"
},
{
"path": "types/react/v17/test/tsx.tsx",
"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"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Critical"
}
],
"reviews": [
{
"type": "approved",
"reviewer": "Andarist",
"date": "2023-05-06T11:15:12.000Z",
"isMaintainer": false
}
],
"mainBotCommentID": 1502029944,
"ciResult": "pass"
} |
|
🔔 @guoshencheng @defnotrobbie @johnnyreilly @bbenezech @pzavolinsky @ericanderson @DovydasNavickas @theruther4d @guilhermehubner @ferdaber @jrakotoharisoa @pascaloliv @Hotell @franklixuefei @Jessidhia @saranshkataria @lukyth @zieka @dancerphil @dimitropoulos @disjukr @vhfmag @hellatan @priyanshurav @Semigradsky — please review this PR in the next few days. Be sure to explicitly select |
There was a problem hiding this comment.
react-blessed relied on a scoped namespace not existing. It wasn't actually using module augmentation. The changes are required to make it work with actual module augmentation since React now has a scoped JSX namespace
typescript-bot
added
the
Unreviewed
|
Re-ping @guoshencheng, @defnotrobbie, @johnnyreilly, @bbenezech, @pzavolinsky, @ericanderson, @DovydasNavickas, @theruther4d, @guilhermehubner, @ferdaber, @jrakotoharisoa, @pascaloliv, @Hotell, @franklixuefei, @Jessidhia, @saranshkataria, @lukyth, @zieka, @dancerphil, @dimitropoulos, @disjukr, @vhfmag, @hellatan, @priyanshurav, @Semigradsky: This PR has been out for over a week, yet I haven't seen any reviews. Could someone please give it some attention? Thanks! |
eps1lon
force-pushed
the
feat/react/scoped-jsx-andarist
branch
from
0181198 to
d6b72cf
Compare
typescript-bot
removed
the
Unreviewed
types/react-blessed/index.d.ts
Outdated
|
|
||
| // augment react JSX when old JSX transform is used | ||
| declare module "react" { | ||
| declare global { |
There was a problem hiding this comment.
Could you elaborate on this change? I wonder about the exact priority for JSX namespaces, like - what's being picked first etc.
Without rechecking this stuff, I can't quickly figure out how this was previously working and how it's working now. One thought train is this:
- previously they declared a JSX namespace within
"react"module and that was picked first, essentially overriding the global JSX namespace - but then... now this declares a global JSX namespace and that merges with the global one declared in React? I'm not sure how to reason about such a merge
There was a problem hiding this comment.
essentially overriding the global JSX namespace
Yep. Replacing would be another word. Just to emphasize this: By declaring a scoped JSX namespace you would disable all declarations relevant to JSX that were done in react. So in that sense you could potentially prevent us from ever shipping a scoped namespace. But I think generally adding stuff via module augmentation should not be considered supported. Otherwise every interface or type we add to the module would be considered a breaking change.
now this declares a global JSX namespace and that merges with the global one declared in React? I'm not sure how to reason about such a merge
The global JSX namespace currently flows into the scoped one i.e. the interfaces in the scoped namespace just extend their global counterparts. We need to do that so that current augmentation still works. If we wouldn't do that, all existing module augmentation (declare global) would stop functioning.
I think this diff is still outdated since you should be able to augment both module 'react' or global and get TS to recognize JSX augmentations. I'll double check.
There was a problem hiding this comment.
Confirmed that this was only needed when we didn't have backwards compat for global module augmentation. Now we do.
To clarify: The global one and augmenting it is deprecated. It should continue to work just like before. In v19 we'll drop it and react-blessed will already worked since they always augmented only the scoped one.
typescript-bot
added
the
Where is GH Actions?
not react though because DefinitelyTyped/DefinitelyTyped#64464
The `JSX` namespace in React was deprecated in [DefinitelyTyped PR #64464](DefinitelyTyped/DefinitelyTyped#64464) and removed in [DefinitelyTyped PR #69022](DefinitelyTyped/DefinitelyTyped#69022). Instead, `React.JSX` should be used.
The `JSX` namespace in React was deprecated in [DefinitelyTyped PR #64464](DefinitelyTyped/DefinitelyTyped#64464) and removed in [DefinitelyTyped PR #69022](DefinitelyTyped/DefinitelyTyped#69022). Instead, `React.JSX` should be used.
5 participants
This allows referring to JSX namespace as
React.JSXinstead of assuming a globalJSXnamespace. Module augmentations of the global namespace flow into the scoped namespace i.e. should still work like before.This deprecates the global
JSXnamespace..In code:
These diffs will work on all latest major types versions e.g. freshly installed
@types/react@latest,@types/react@^17.0.0etc.In a future major we'll drop the global namespace.
Potential breaking changes
If you added a
JSXnamespace tomodule "react"you need to make sure this augmentation passes declaration merging i.e. just adds properties to theJSXnamespace.We don't consider these breaking changes under SemVer i.e. don't warrant a major release.
Otherwise each interface or type we add to React would warrant a major release.
Closes #52321
TODO: