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.

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

feat: remove remaining React imports #9907

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

nickserv
Copy link
Contributor

@nickserv nickserv commented Oct 25, 2020

Follows up on #9853 and removes remaining React imports, as React 17 is supported and #9734 has been merged.

@nickserv
Copy link
Contributor Author

nickserv commented Oct 25, 2020

A smoke test has the ESLint error 'React' is not defined react/jsx-no-undef in src/index.tsx. It seems like this is only an issue with TypeScript. Should we disable react/jsx-no-undef with TS like we already do with no-undef?

https://dev.azure.com/facebook/create-react-app/_build/results?buildId=2072&view=logs&j=2a983690-2011-537a-f1b1-93b016d28b7e&t=6a887007-3b70-5b2f-836b-0f0b425fc73a

@MichaelDeBoey
Copy link
Contributor

Shouldn't this be handled automatically by eslint-plugin-react when React 17 is detected? 🤔

CC: @yannickcr

@yannickcr
Copy link

@MichaelDeBoey For eslint-plugsin-react and React 17 you only have to disable the react/jsx-uses-react and react/react-in-jsx-scope rules: https://reactjs.org/blog/2020/09/22/introducing-the-new-jsx-transform.html#eslint

For this case I'd say the React import should not be removed from here https://github.com/facebook/create-react-app/pull/9907/files#diff-64c681ba8886069f7e1af46ff0db7eb161244eb14a4a7c780d83eaae6c126cbf since it is still used for React.StrictMode

@n3tr
Copy link
Contributor

n3tr commented Oct 27, 2020

The current version of TypeScript (4.0.x) doesn't support the new JSX transform (introducing in 4.1).

Removing import React from the typescript template might break the app unless the user installs [email protected].

There is code checking the ts version and switch to use new jsx configuration

@nickserv
Copy link
Contributor Author

nickserv commented Oct 29, 2020

@MichaelDeBoey @yannickcr Is it safe to remove those rules from eslint-config-react-app? It could cause confusion for any external users that still aren't using the new JSX transform, but CRA 4 users should be fine either way.

@n3tr There is an issue with TS 4.1 beta support in CRA 4 (#9868) that makes upgrading tricky anyway, and by the time that fix is published TS 4.1 may already be stable.

@nickserv
Copy link
Contributor Author

TypeScript 4.1 is stable and #9868 has been closed, so this should be ready to merge once the review suggestions are resolved.

@stale
Copy link

stale bot commented Dec 25, 2020

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Dec 25, 2020
@nickserv
Copy link
Contributor Author

I have an unresolved question:

Is it safe to remove those rules from eslint-config-react-app? It could cause confusion for any external users that still aren't using the new JSX transform, but CRA 4 users should be fine either way.

@stale
Copy link

stale bot commented Jan 9, 2022

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Jan 9, 2022
@MichaelDeBoey
Copy link
Contributor

We should still merge this one

@nickserv
Copy link
Contributor Author

nickserv commented Jun 8, 2022

This is ready to review again, but I'd still like to know the plan with the lint config

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

Successfully merging this pull request may close these issues.

5 participants