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

bun is reported by noUndeclaredDependencies #2074

Open
1 task done
risu729 opened this issue Mar 13, 2024 · 9 comments
Open
1 task done

bun is reported by noUndeclaredDependencies #2074

risu729 opened this issue Mar 13, 2024 · 9 comments
Labels
A-Linter Area: linter L-JavaScript Language: JavaScript and super languages S-Enhancement Status: Improve an existing feature

Comments

@risu729
Copy link
Contributor

risu729 commented Mar 13, 2024

Environment information

CLI:
  Version:                      1.6.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:           "v21.6.1"
  JS_RUNTIME_NAME:              "node"
  NODE_PACKAGE_MANAGER:         "bun/1.0.29"

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

Linter:
  Recommended:                  false
  All:                          true
  Rules:
Workspace:
  Open Documents:               0

Rule name

noUndeclaredDependencies

Playground link

https://biomejs.dev/playground/?lintRules=all&code=aQBtAHAAbwByAHQAIAB7ACAAcwBsAGUAZQBwACAAfQAgAGYAcgBvAG0AIAAiAGIAdQBuACIACgAKAHMAbABlAGUAcAAoADEAKQA%3D&jsx=false

The playground does not support package.json, so it may not be meaningful.

Expected result

Code

import { sleep } from "bun"

It should not report any error, but reports bun by noUndeclaredDependencies.

Solution

// Ignore relative path imports
// Ignore imports using a protocol such as `node:`, `bun:`, `jsr:`, `https:`, and so on.
if text.starts_with('.') || text.contains(':') {
return None;
}

To add bun here should fix this issue. I would be happy to open a PR.

Code of Conduct

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

Sec-ant commented Mar 13, 2024

Share my two cents here.

To be honest I don't think we should hardcode dependencies in this rule. The bun module is declared in @types/bun and that's why typescript is ok about it. But I don't have any better idea either to make this rule smarter enough to recognize declared modules buried inside node_modules or other typeRoots. And if it is a JS project, we don't even have these places to look for.

A whitelist in the rule option seems a go to me.

@risu729
Copy link
Contributor Author

risu729 commented Mar 13, 2024

I agree with your idea.

To me, it would be helpful to add an option and also detect @types/bun from package.json.

@arendjr
Copy link
Contributor

arendjr commented Mar 13, 2024

Maybe using @types/bun would become feasible when we have our own type inference, but if we blindly accept any package name after @types/ I’m afraid we’ll get false negatives too. Not all type packages are used to define import modules, some only define globals/namespaces.

For now I agree an allowlist is the best way forward.

@ematipico
Copy link
Member

I think this is the correct usage of the rule for this particular case.

The user should either use the global Bun or import the method from bun:.

If both aren't possible, then I think the rule is acceptable. Although, I don't know bun very well

@risu729
Copy link
Contributor Author

risu729 commented Mar 13, 2024

Thank you for your comments!

For me, it is unintuitive that the rule doesn't report bun: modules but reports bun.

I understand that using the Bun global object is better for readability, but I think this rule aims to avoid unlisted dependencies in package.json.

@ematipico
Copy link
Member

but I think this rule aims to avoid unlisted dependencies in package.json.

I think I misunderstood the issue. Is bun inside your package.json?

@Conaclos
Copy link
Member

As a mid-solution (only work for project using TS), we could rewrite @types/<path> to <path> and use as actual dependency. However, this can be unsafe for other modules.

@risu729
Copy link
Contributor Author

risu729 commented Mar 13, 2024

but I think this rule aims to avoid unlisted dependencies in package.json.

I think I misunderstood the issue. Is bun inside your package.json?

Sorry, your understanding should be right.

I meant to say that we do not need to add runtime to dependencies, so should not be reported.

@Conaclos Conaclos added A-Linter Area: linter L-JavaScript Language: JavaScript and super languages S-Enhancement Status: Improve an existing feature labels Mar 13, 2024
@Conaclos Conaclos changed the title 💅 bun is reported by noUndeclaredDependencies bun is reported by noUndeclaredDependencies Mar 13, 2024
@Sec-ant
Copy link
Contributor

Sec-ant commented Mar 13, 2024

As a mid-solution (only work for project using TS), we could rewrite @types/<path> to <path> and use as actual dependency. However, this can be unsafe for other modules.

I not in favor of this solution because this can introduce many false positives and false negatives that may confuse users and eventually may make this rule useless.

TypeScript uses types and typeRoots fields to decide which type declaration packages it should check. And module declarations can also be added in a simple user-created .d.ts file.

It would be much safer and configurable to let users set the allow list explicitly. And in the meantime before type inference is a thing, Biome won't be blamed :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Linter Area: linter L-JavaScript Language: JavaScript and super languages S-Enhancement Status: Improve an existing feature
Projects
None yet
Development

No branches or pull requests

5 participants