Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Replace JSX.Element with React.JSX.Element #38615

Closed
wants to merge 1 commit into from
Closed

Replace JSX.Element with React.JSX.Element #38615

wants to merge 1 commit into from

Conversation

retyui
Copy link
Contributor

@retyui retyui commented Jul 25, 2023

Summary:

the global JSX namespace was depreacted in react 18:

declare global {
    /**
     * @deprecated Use `React.JSX` instead of the global `JSX` namespace.
     */
    namespace JSX {

Changelog:

[GENERAL] [CHANGED] - Replace JSX.Element with React.JSX.Element in App.tsx template

Test Plan:

Before

Screenshot 2023-07-25 at 14 11 59

After

Screenshot 2023-07-25 at 14 12 12

@retyui
Replace JSX.Element with React.JSX.Element
db6a859 
the global `JSX` namespace was depreacted:

```tsx
declare global {
    /**
     * @deprecated Use `React.JSX` instead of the global `JSX` namespace.
     */
    namespace JSX {
```
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,843,442 -1
android hermes armeabi-v7a 8,152,596 -2
android hermes x86 9,349,234 -1
android hermes x86_64 9,191,953 -3
android jsc arm64-v8a 9,456,861 -2
android jsc armeabi-v7a 8,637,997 +0
android jsc x86 9,539,949 -1
android jsc x86_64 9,783,246 -2

Base commit: e64756a
Branch: main

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Jul 25, 2023
@NickGerleman
Copy link
Contributor

I can see the PR where this happened in DefinitelyTyped/DefinitelyTyped#64464, but it seems kinda wild to me that the old version was deprecated. It breaks the conventions of a lot of the documentation (including TS itself) and existing code out there.

I would tend to gravitate to keeping it as is, while “JSX.Element” is what TypeScript docs lead towards. https://www.typescriptlang.org/docs/handbook/jsx.html

NickGerleman
NickGerleman requested changes Jul 25, 2023
View reviewed changes
Copy link
Contributor

@NickGerleman NickGerleman left a comment

Choose a reason for hiding this comment

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

Every other typings package on DT seems to use raw “JSX.Element” as well.

I’m not sure if the deprecation was meant to be internal to typings, or maybe an tad optimistic, but it doesn’t really feel idiomatic to use this new form when all of the documentation, types, and code out there does not use it.

@NickGerleman
Copy link
Contributor

Seeing this create a deprecation warning is worrying though. I will take a closer look at the original PR to DT.

@retyui
Copy link
Contributor Author

retyui commented Jul 25, 2023

More info you can find in the original issue: DefinitelyTyped/DefinitelyTyped#52321

NickGerleman added a commit to NickGerleman/DefinitelyTyped that referenced this pull request Jul 25, 2023
@NickGerleman
Undeprecate Global React JSX Namespace
b2bc282 
DefinitelyTyped#64464 (landed in semver patch release `@types/react@18.2.6`) scopes the JSX namespace as `React.JSX` to avoid collisions with multiple libraries working in the JSX namespace.

This change also deprecated the existing JSX namespace, so existing usages of `JSX.Element`, etc are flagged in-editor.

<img width="686" alt="Screenshot 2023-07-25 at 14 11 59" src="https://github.com/facebook/react-native/assets/4661784/531a4f88-8090-43aa-86d6-4af595d4cb5d"> 

This has a pretty drastic effect of making every previous non-anonymous function component, with an explicit return type, now fire deprecation warnings. E.g. this patch update now makes the out-of-the-box React native template create deprecation warnings. facebook/react-native#38615

In DefinitelyTyped itself, the hundreds of packages using `JSX.Element` outside of the direct React typings were not updated. TypeScript's [own documentation](https://www.typescriptlang.org/docs/handbook/jsx.html) also still suggests using `JSX.Element`. This is echoed by a platitude of existing documentation, and googling for `React.JSX.Element` will just bring up information about `JSX.Element`.

Due to the widespread nature of the breakage here (especially in a patch release), and disconnect with existing code, documentation, and resources, this change removes the `@deprecated` annotation. I think there might still be a possible path to discouraging usage, but there needs to be a lot of legwork to get there in a way which isn't disruptive.
@NickGerleman NickGerleman mentioned this pull request Jul 25, 2023
Closed
@NickGerleman
Copy link
Contributor

I propsed DefinitelyTyped/DefinitelyTyped#66171 to un-deprecate the previous namespace. I think the change to deprecate was too disruptive as it was made.

NickGerleman
NickGerleman approved these changes Jul 28, 2023
View reviewed changes
Copy link
Contributor

@NickGerleman NickGerleman left a comment

Choose a reason for hiding this comment

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

I really dislike the path taken by this deprecation, but now that this API is out in the wild it does seem like we should be using it.

@facebook-github-bot
Copy link
Contributor

@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Jul 29, 2023
@facebook-github-bot
Copy link
Contributor

@NickGerleman merged this pull request in 1383a59.

billnbell pushed a commit to billnbell/react-native that referenced this pull request Jul 29, 2023
@retyui @billnbell3
Replace JSX.Element with React.JSX.Element (facebook#38615)
dccce69 
Summary:
the global `JSX` namespace was depreacted in react 18:

```tsx
declare global {
    /**
     * deprecated Use `React.JSX` instead of the global `JSX` namespace.
     */
    namespace JSX {
```

## Changelog:

[GENERAL] [CHANGED] - Replace `JSX.Element` with `React.JSX.Element` in `App.tsx` template

Pull Request resolved: facebook#38615

Test Plan:
Before

<img width="686" alt="Screenshot 2023-07-25 at 14 11 59" src="https://github.com/facebook/react-native/assets/4661784/531a4f88-8090-43aa-86d6-4af595d4cb5d">

After

<img width="445" alt="Screenshot 2023-07-25 at 14 12 12" src="https://github.com/facebook/react-native/assets/4661784/3b427938-2768-4131-b77a-62045e5b8d08">

Reviewed By: rshest

Differential Revision: D47873435

Pulled By: NickGerleman

fbshipit-source-id: c8a9e0e8e96a54c6ee66fcae2392e0d20d20d026
blakef pushed a commit to blakef/template that referenced this pull request Feb 28, 2024
@retyui @facebook-github-bot
Replace JSX.Element with React.JSX.Element (#38615)
502ed64 
Summary:
the global `JSX` namespace was depreacted in react 18:

```tsx
declare global {
    /**
     * deprecated Use `React.JSX` instead of the global `JSX` namespace.
     */
    namespace JSX {
```

## Changelog:

[GENERAL] [CHANGED] - Replace `JSX.Element` with `React.JSX.Element` in `App.tsx` template

Pull Request resolved: facebook/react-native#38615

Test Plan:
Before

<img width="686" alt="Screenshot 2023-07-25 at 14 11 59" src="https://github.com/facebook/react-native/assets/4661784/531a4f88-8090-43aa-86d6-4af595d4cb5d">

After

<img width="445" alt="Screenshot 2023-07-25 at 14 12 12" src="https://github.com/facebook/react-native/assets/4661784/3b427938-2768-4131-b77a-62045e5b8d08">

Reviewed By: rshest

Differential Revision: D47873435

Pulled By: NickGerleman

fbshipit-source-id: c8a9e0e8e96a54c6ee66fcae2392e0d20d20d026

Original: facebook/react-native@1383a59
blakef pushed a commit to react-native-community/template that referenced this pull request Feb 29, 2024
@retyui @facebook-github-bot
Replace JSX.Element with React.JSX.Element (#38615)
2e5274c 
Summary:
the global `JSX` namespace was depreacted in react 18:

```tsx
declare global {
    /**
     * deprecated Use `React.JSX` instead of the global `JSX` namespace.
     */
    namespace JSX {
```

## Changelog:

[GENERAL] [CHANGED] - Replace `JSX.Element` with `React.JSX.Element` in `App.tsx` template

Pull Request resolved: facebook/react-native#38615

Test Plan:
Before

<img width="686" alt="Screenshot 2023-07-25 at 14 11 59" src="https://github.com/facebook/react-native/assets/4661784/531a4f88-8090-43aa-86d6-4af595d4cb5d">

After

<img width="445" alt="Screenshot 2023-07-25 at 14 12 12" src="https://github.com/facebook/react-native/assets/4661784/3b427938-2768-4131-b77a-62045e5b8d08">

Reviewed By: rshest

Differential Revision: D47873435

Pulled By: NickGerleman

fbshipit-source-id: c8a9e0e8e96a54c6ee66fcae2392e0d20d20d026

Original-Commit: facebook/react-native@1383a59
blakef pushed a commit to react-native-community/template that referenced this pull request Feb 29, 2024
@retyui @facebook-github-bot
Replace JSX.Element with React.JSX.Element (#38615)
b9b67d0 
Summary:
the global `JSX` namespace was depreacted in react 18:

```tsx
declare global {
    /**
     * deprecated Use `React.JSX` instead of the global `JSX` namespace.
     */
    namespace JSX {
```

## Changelog:

[GENERAL] [CHANGED] - Replace `JSX.Element` with `React.JSX.Element` in `App.tsx` template

Pull Request resolved: facebook/react-native#38615

Test Plan:
Before

<img width="686" alt="Screenshot 2023-07-25 at 14 11 59" src="https://github.com/facebook/react-native/assets/4661784/531a4f88-8090-43aa-86d6-4af595d4cb5d">

After

<img width="445" alt="Screenshot 2023-07-25 at 14 12 12" src="https://github.com/facebook/react-native/assets/4661784/3b427938-2768-4131-b77a-62045e5b8d08">

Reviewed By: rshest

Differential Revision: D47873435

Pulled By: NickGerleman

fbshipit-source-id: c8a9e0e8e96a54c6ee66fcae2392e0d20d20d026

Original-Commit: facebook/react-native@1383a59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NickGerleman NickGerleman NickGerleman approved these changes

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@retyui @analysis-bot @NickGerleman @facebook-github-bot