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

🐛 noUnusedImports removes React import unsafely #571

Closed
1 task done
EvHaus opened this issue Oct 21, 2023 · 6 comments · Fixed by #2306
Closed
1 task done

🐛 noUnusedImports removes React import unsafely #571

EvHaus opened this issue Oct 21, 2023 · 6 comments · Fixed by #2306

Comments

@EvHaus
Copy link

EvHaus commented Oct 21, 2023

Environment information

CLI:
  Version:                      1.3.1
  Color support:                true

Platform:
  CPU Architecture:             x86_64
  OS:                           linux

Environment:
  BIOME_LOG_DIR:                unset
  NO_COLOR:                     unset
  TERM:                         "xterm-256color"
  JS_RUNTIME_VERSION:           "v20.6.1"
  JS_RUNTIME_NAME:              "node"
  NODE_PACKAGE_MANAGER:         "pnpm/8.9.2"

Biome Configuration:
  Status:                       Loaded successfully
  Formatter disabled:           false
  Linter disabled:              false
  Organize imports disabled:    false
  VCS disabled:                 true

Workspace:
  Open Documents:               0

What happened?

The nursery.noUnusedImports rule currently removes the import React from 'react' import in React projects (.tsx/.jsx files) when it's not directly used. This is not a safe thing to do. Although some postprocessors (like Next.js) handle this fine, others like jest and vitest will fail with:

ReferenceError: React is not defined
 ❯ ui/Alert/Alert.test.tsx:14:32
     12|
     13|  it('should render with only a title', () => {
     14|   const { getByText } = render(<Alert title='Hello World' />);
       |                                ^
     15|   expect(getByText('Hello World')).toBeDefined();
     16|  });

This is because JSX syntax is transpiled to React.createClass under-the-hood.

Expected result

Ideally Biome would leave import React from 'react' imports alone if there is JSX syntax in the file. If that's not possible, maybe it's better to add an exception to the react module for now?

Code of Conduct

  • I agree to follow Biome's Code of Conduct
@nissy-dev
Copy link
Contributor

nissy-dev commented Oct 22, 2023

Although some postprocessors (like Next.js) handle this fine, others like jest and vitest will fail with:

In many cases, this is due to a lack of bundler or transpiler settings.

If you use vitest with React, you need to use vitejs/plugin-react
ref: vitest-dev/vitest#1572 (comment)

If you use jest, you need to set some options for transpiler

Babel: https://babeljs.io/docs/babel-preset-react#both-runtimes
SWC: https://swc.rs/docs/configuration/compilation#jsctransformreactruntime

So, as you say, removing react is not safe and has side effects,
but there are other ways to fix it without waiting for updating our tools.

@Conaclos
Copy link
Member

I am unsure about the course of action here. I think we will wait to gather more feedbacks.

@EvHaus
Copy link
Author

EvHaus commented Oct 22, 2023

If you use vitest with React, you need to use vitejs/plugin-react

Ah! That's a great point. That works for me.

I think I'll close this issue now as my specific problem is resolved with that solution.

@EvHaus EvHaus closed this as completed Oct 22, 2023
@martinuncountable
Copy link

@Conaclos we hit this today too. It is safer for us not to be forced to remove the imports. Can there be an option setting for the rule to ignore this import?

@martinuncountable
Copy link

@Conaclos we hit this today too. It is safer for us not to be forced to remove the imports. Can there be an option setting for the rule to ignore this import?

I am going to enable jsx=automatic in esbuild and work around this. We dont have any other imports to ignore AFAIK at this time.

@ipanasenko
Copy link
Contributor

We have a non-editable company-wide webpack and TS configs, and we can't remove React import.
Is there a possibility to add an option to preserve React imports?

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

Successfully merging a pull request may close this issue.

5 participants