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

Handle new JSX transform #2834

Closed
mrmckeb opened this issue Oct 18, 2020 · 9 comments
Closed

Handle new JSX transform #2834

mrmckeb opened this issue Oct 18, 2020 · 9 comments

Comments

@mrmckeb
Copy link

mrmckeb commented Oct 18, 2020

We've added support for the new JSX transform into Create React App (facebook/create-react-app#9645, facebook/create-react-app#9788), and we've been discussing how best to support this in our ESLint config.

I'm not sure if this is a good idea, and wanted some thoughts. I wondered if we should update jsx-uses-react to warn when using a React version that ships with the new transform, and do one of the following:

  • Add an autofix (remove all React imports).
  • Recommend running the codemod.

For now, we're:

  • Auto-detecting support for the new transform (see base.js#L14-L21).
  • Updating rules based on that detection, leaving jsx-uses-react on to avoid confusion (see base.js#L52-L58).
@ljharb
Copy link
Member

ljharb commented Oct 18, 2020

If the jsx-uses-react rule is simply disabled, a “no unused imports” or “no unused variables” rule would already catch an unnecessary React import. When there’s a React version that doesn’t allow the current jsx transform, then the jsx-uses-react rule can be updated to throw an error, but otherwise I’m not sure any change is needed in this plugin.

@mrmckeb
Copy link
Author

mrmckeb commented Oct 19, 2020

I was thinking more about explaining why React is no longer needed to be imported.

I agree 100% that you could just rely on a no-unused variable, and as I said I was just interested in thoughts. As the community transitions, some members will obviously be confused... but a change to this plugin may not help, you're right.

We can close this off for now, again I was just interested in thoughts at this stage - thanks for your time!

@mrmckeb mrmckeb closed this as completed Oct 19, 2020
@cdoublev
Copy link

cdoublev commented Nov 1, 2020

Hello,

It would be nice to detect "runtime": "automatic" for the babel/preset-react option and disable errors from react-in-jsx-scope when applicable, since this rule is recommended.

@ljharb
Copy link
Member

ljharb commented Nov 1, 2020

I’d rather not couple this plugin to babel so tightly.

@cdoublev
Copy link

cdoublev commented Nov 1, 2020

Obviously. Though I predict that after the release of Babel 8, many people will ask the same thing (or to remove it from recommended rules).

@mrmckeb
Copy link
Author

mrmckeb commented Nov 2, 2020

Perhaps the React version detection in this plugin could alter behaviour of the related rule?

@ljharb
Copy link
Member

ljharb commented Nov 2, 2020

Using React does not guarantee you're using JSX, or Babel, or transpiling at all, so I'm afraid not.

@cyrus-za
Copy link

cyrus-za commented Apr 1, 2021

I disagree that no-unused-vars would catch it.

If someone writes

import React from 'react'

export const Header: React.FC = () => { }

Then the rule will not warn against it and then when the new react version launches, someone will need to refactor the entire codebase to remove default imports that could have been prevented by a eslint rule.

I know there's a codemod but it does not catch all scenarios.

@ljharb
Copy link
Member

ljharb commented Apr 1, 2021

@cyrus-za in that case, the React import is strictly necessary, for the type. There’s nothing wrong with having it there.

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

No branches or pull requests

4 participants