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 supports custom jsxFactory #3682

Open
1 task done
ahaoboy opened this issue Aug 19, 2024 · 11 comments · May be fixed by #3761
Open
1 task done

🐛 noUnusedImports supports custom jsxFactory #3682

ahaoboy opened this issue Aug 19, 2024 · 11 comments · May be fixed by #3761
Assignees
Labels
A-Linter Area: linter S-Feature Status: new feature to implement S-Help-wanted Status: you're familiar with the code base and want to help the project

Comments

@ahaoboy
Copy link

ahaoboy commented Aug 19, 2024

Environment information

CLI:
  Version:                      1.8.3
  Color support:                true

Platform:
  CPU Architecture:             x86_64
  OS:                           windows

Environment:
  BIOME_LOG_DIR:                unset
  NO_COLOR:                     unset
  TERM:                         "xterm-256color"
  JS_RUNTIME_VERSION:           "v22.6.0"
  JS_RUNTIME_NAME:              "node"
  NODE_PACKAGE_MANAGER:         "pnpm/9.6.0"

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

Workspace:
  Open Documents:               0

What happened?

https://github.com/ahaoboy/biome-react-jsxFactory

When using a custom renderer, after turning on noUnusedImports, the jsxFactory field configured in tsconfig is not correctly recognized

For example, in the following code, only variable b should not be used.
However, biome will mark both $ and b as unused.

    "jsx": "react",
    "jsxFactory": "$",
    "jsxFragmentFactory": "$"
import { $, b } from "./react"
function App() {
  return <div />
}
export default <App />

Expected result

biome can handle custom jsxFactory correctly
For the above code, only variable b should not be used.

Code of Conduct

  • I agree to follow Biome's Code of Conduct
@ematipico ematipico added A-Linter Area: linter S-Feature Status: new feature to implement labels Aug 19, 2024
@ahaoboy
Copy link
Author

ahaoboy commented Aug 19, 2024

Is it possible to add a separate configuration? For a very large monorepo, there may not be a tsconfig in the root directory

  "javascript": {
    "jsxRuntime": {
      "jsx": "react",
      "jsxFactory": "$",
      "jsxFragmentFactory": "$",
    },
  },

@ematipico
Copy link
Member

PRs are welcome if anyone wants to help

@ematipico ematipico added the S-Help-wanted Status: you're familiar with the code base and want to help the project label Aug 28, 2024
@h-a-n-a
Copy link
Contributor

h-a-n-a commented Aug 29, 2024

I'm new to biome.js but I'm very glad to help if you can bear with me taking a bit longer to fix this ❤️

@h-a-n-a
Copy link
Contributor

h-a-n-a commented Aug 29, 2024

@ematipico Hi! I was wondering if we can support:

{
  "javascript": {
    "jsxRuntime": "reactClassic",
    "jsxFactory": "$",
    "jsxFragmentFactory": "$"
  }
}

Note: jsxFactory or jsxFragmentFactory will only be used when jsxRuntime is set to "reactClassic".

If we were to support struct type of jsxRuntime, this would cause a breaking change to config file. Otherwise, we may need to support either type in deserialization for jsxRuntime, which could probably make JSON schema hard to maintain as well.
However, flattening these options do not cause any of the problems above.

Do you have any bias on this one?

@ahaoboy
Copy link
Author

ahaoboy commented Aug 30, 2024

Considering that different subprojects in monorepo may have different configurations, it would be nice to read the configurations from the tsconfig files of the subprojects, but there is extends field in tsconfig, and it is not clear if biome supports it.

@ematipico
Copy link
Member

@ahaoboy if a project doesn't use TypeScript, how does someone provide those values?

@ahaoboy
Copy link
Author

ahaoboy commented Aug 30, 2024

@ahaoboy if a project doesn't use TypeScript, how does someone provide those values?

Yes, this is why I want to add a configuration. Can we use the default configuration for projects without tsconfig?

import React from 'react'

@ematipico
Copy link
Member

@h-a-n-a Your proposal sounds reasonable. I suppose we could add an ad-hoc validation that emits an error/warning in case of incorrect configuration. Feel free to start the implementation as you see fit

@ahaoboy thank you for your answer

@h-a-n-a
Copy link
Contributor

h-a-n-a commented Aug 30, 2024

@ematipico Hi! I encountered some problem.

Background

I dug into the codebase. I found out the original implementation is a little bit simple:
https://github.com/arendjr/biome/blob/67d03778769c9b944b03575032f4bdbcabec27c9/crates/biome_js_analyze/src/lint/correctness/no_unused_imports.rs#L99-L103
It completely ignored the case if jsxRuntime is set to ReactClassic and if it's a global react import statement.

The following module does not complain any error even though the identifier binding React is not being used:

import React from "react"

The other issue I encountered is if jsxRuntime is set to ReactClassic, and with the following code:

function App() {
  return <><div>abc</div></>
}

export default <App />

Biome does not complain anything (note that React is not being imported in this case). However, as per the jsxRuntime section of the document:

Corresponds to the react value for the jsx option in TypeScript’s tsconfig.json

And at the same time, TypeScript compiler does complain as follows:

Cannot find name 'React'.

So I believe there does have something to improve and I want to focus on this issue first.

Possible solution

I would like to improve the binding.
For example, for jsxRuntime being set to ReactClassic and jsxFactory being set to h, both of the following examples should not complain:

// 1
const h = () => {}
const Element = <div></div>

// 2
import { h } from "preact"
const Element = <div></div>

Also, jsx runtime should be scope aware, with the same configuration above, the following example should complain:

function App() {
  const h = () => {}
  return <div /> // 1
}

export default <App /> // 2 👈 Cannot find binding `h`

Note 1 should not complain as h is being defined locally, however 2 does not have any identifier binding defined.

Question

I took a look at the SemanticEventExtractor and I was thinking of every time a SyntaxKind::JSX_ELEMENT is visited, a reference should be added to the scope so that I can get the reference in no_used_imports with binding.all_references without changing the plugin code. However, maybe I'm taking this wrong, because of every BindingName and Reference is from the raw node in roman, there's no place to add a synthetic binding.

I don't know if I'm heading towards the right direction. Would you please help me with a little guidance?
Thanks for reading and being patient with me!

@ematipico
Copy link
Member

It completely ignored the case if jsxRuntime is set to ReactClassic and if it's a global react import statement.

I believe it's intended, because in "classic" react, the old way, the global React was mandatory. This code should pass, because the old version of babel JSX transformer for react requires it:

import React from "react"

export function App() {
	return <p>hello</p>
}

However, a more modern version of the babel JSX transformer doesn't require it anymore. In lue to that, we decided to ignore the import in case we have a classic (old) transformer.

So I believe there does have something to improve and I want to focus on this issue first.

Please do. Consider that, at the moment, we can't read the tsconfig.json yet. It's coming soon.


Regarding your question, I believe the semantic model already does that 🤔

@h-a-n-a
Copy link
Contributor

h-a-n-a commented Aug 30, 2024

Regarding your question, I believe the semantic model already does that 🤔

@ematipico AFAIK, they only do bindings for cases like:

const App = () => {}
<App /> // App usage

But instead, I'd like to add references to jsxFactory and jsxFragmentFactory.

Ref: https://github.com/arendjr/biome/blob/01a80a47afae76d80f847971a979b3315d3eda67/crates/biome_js_semantic/src/events.rs#L747

@h-a-n-a h-a-n-a linked a pull request Sep 2, 2024 that will close this issue
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Linter Area: linter S-Feature Status: new feature to implement S-Help-wanted Status: you're familiar with the code base and want to help the project
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants