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

💅 noUndeclaredDependencies should also validate peerDependencies? #2122

Closed
1 task done
ipanasenko opened this issue Mar 18, 2024 · 4 comments · Fixed by #2128
Closed
1 task done

💅 noUndeclaredDependencies should also validate peerDependencies? #2122

ipanasenko opened this issue Mar 18, 2024 · 4 comments · Fixed by #2128
Labels
A-Analyzer Area: analyzer good first issue Good for newcomers S-Bug-confirmed Status: report has been confirmed as a valid bug

Comments

@ipanasenko
Copy link
Contributor

Environment information

CLI:
  Version:                      1.6.1
  Color support:                true

Platform:
  CPU Architecture:             aarch64
  OS:                           macos

Environment:
  BIOME_LOG_DIR:                unset
  NO_COLOR:                     unset
  TERM:                         "xterm-256color"
  JS_RUNTIME_VERSION:           "v18.19.1"
  JS_RUNTIME_NAME:              "node"
  NODE_PACKAGE_MANAGER:         "npm/10.2.4"

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

Linter:
  Recommended:                  true
  All:                          false
  Rules:                        a11y/noSvgWithoutTitle = "off"
                                a11y/useKeyWithClickEvents = "off"
                                a11y/useKeyWithMouseEvents = "off"
                                a11y/useValidAnchor = "off"
                                complexity/noUselessLoneBlockStatements = "error"
                                correctness/noInvalidUseBeforeDeclaration = "off"
                                correctness/noNewSymbol = "error"
                                correctness/noUnusedPrivateClassMembers = "error"
                                correctness/noUnusedVariables = "error"
                                correctness/useExhaustiveDependencies = "off"
                                correctness/useHookAtTopLevel = "error"
                                nursery/noDoneCallback = "error"
                                nursery/noExportsInTest = "error"
                                nursery/noSemicolonInJsx = "error"
                                nursery/noUndeclaredDependencies = "error"
                                nursery/noUselessTernary = "error"
                                performance/noDelete = "off"
                                style/noNegationElse = "error"
                                style/noNonNullAssertion = "off"
                                style/noRestrictedGlobals = "error"
                                style/noShoutyConstants = "error"
                                style/noUnusedTemplateLiteral = "off"
                                style/useBlockStatements = "error"
                                style/useCollapsedElseIf = "error"
                                style/useForOf = "error"
                                style/useImportType = "off"
                                style/useNodejsImportProtocol = "error"
                                style/useNumberNamespace = "error"
                                style/useShorthandAssign = "error"
                                style/useShorthandFunctionType = "error"
                                style/useSingleCaseStatement = "error"
                                suspicious/noConsoleLog = "error"
                                suspicious/noRedeclare = "off"

Workspace:
  Open Documents:               0

Rule name

lint/nursery/noUndeclaredDependencies

Playground link

none

Expected result

Does not mark dependencies from peerDependencies as undeclared

Code of Conduct

  • I agree to follow Biome's Code of Conduct
@arendjr
Copy link
Contributor

arendjr commented Mar 18, 2024

I agree this would make sense indeed.

@arendjr arendjr added good first issue Good for newcomers S-Bug-confirmed Status: report has been confirmed as a valid bug A-Analyzer Area: analyzer labels Mar 18, 2024
@Sec-ant
Copy link
Member

Sec-ant commented Mar 18, 2024

I'd like to work on this and #2123.

But I have some questions on the future of this rule and maybe I can address them altogether if I can get a definite answer.

  1. Should we also support optionalDependencies defined in package.json?

  2. Should we support subpath imports defined in package.json? If the answer is yes, should we write our own pattern matching logic or just use oxc_resolver to match the keys of imports ? (unlike dependencies, asterisks are allowed in imports keys for glob-like matching)

  3. oxc_resolver is also able to parse paths (a kind of import alias) defined in a tsconfig.json file. Should we use it to support them as well in this rule? The main problem would be that oxc_resolver requires the path of the tsconfig.json file as an input, which may become an issue in projects where there're multiple levels of folders and multiple configs. There's a package called tsconfck in JS to handle tsconfig.json resolution and parsing as it's solely purpose. We can use oxc_resolver for parsing, but for resolution, I didn't find any in Rust. I'm not sure if finding the closest tsconfig.json would be the correct solution.

@arendjr
Copy link
Contributor

arendjr commented Mar 18, 2024

Should we also support optionalDependencies defined in package.json?

Yes.

Should we support subpath imports defined in package.json?

Yes. But I think we may also want to generalize this to include support for import maps (also supported by Deno). So I would suggest implementing this in one or two separate PRs (depending on how much can be shared between the NPM-based version and WHATWG import maps). Let's first focus on a version without subpath imports.

If the answer is yes, should we write our own pattern matching logic or just use oxc_resolver to match the keys of imports ? (unlike dependencies, asterisks are allowed in imports keys for glob-like matching)

Using oxc_resolver sounds like a good idea, but I'm not sure to which extend it's applicable here. Maybe @ematipico knows this :)

oxc_resolver is also able to parse paths (a kind of import alias) defined in a tsconfig.json file. Should we use it to support them as well in this rule?

I would say yes, in theory. But I think it's best to wait with this until the type inference is underway. By then we will probably have some more infrastructure to support handling tsconfig.json files.

@ematipico
Copy link
Member

ematipico commented Mar 18, 2024

The handling of tsconfig.json and multiple folders - AKA monorepos with multiple tsconfig.json and package.json - should be handled in a different PR with some context, maybe with some discussion on how to handle it. Mostly because we have to get it right in CLI and LSP. oxc_resolver is limited unfortunately, I hope to find a way to make it better but I'd need some time to think about it.

Plus, as a side note, this rule shouldn't check for installed dependencies. Of course, the scope can change, after all it's a nursery rule. Let's see how it goes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Analyzer Area: analyzer good first issue Good for newcomers S-Bug-confirmed Status: report has been confirmed as a valid bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants