biome icon indicating copy to clipboard operation
biome copied to clipboard

💅 `noUndeclaredDependencies`: not resolving tsconfig paths nor package subpaths

Open kvnwolf opened this issue 1 year ago • 12 comments
trafficstars

Environment information

CLI:
  Version:                      1.6.0
  Color support:                true

Platform:
  CPU Architecture:             aarch64
  OS:                           macos

Environment:
  BIOME_LOG_DIR:                unset
  NO_COLOR:                     unset
  TERM:                         "xterm-256color"
  JS_RUNTIME_VERSION:           "v20.10.0"
  JS_RUNTIME_NAME:              "node"
  NODE_PACKAGE_MANAGER:         "bun/1.0.30"

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

Linter:
  Recommended:                  false
  All:                          true
  Rules:                        nursery/all = true
                                nursery/useSortedClasses = {"level":"error","options":{"attributes":["class","className","className","classList"],"functions":["clsx","cva","tw","cn"]}}
                                style/all = true
                                style/noImplicitBoolean = "off"
                                style/useBlockStatements = "off"
                                style/useNamingConvention = "off"

Workspace:
  Open Documents:               0

Rule name

noUndeclaredDependencies

Playground link

N/A

Expected result

I couldn't replicate the issue on the playground because I couldn't find a way to add a tsconfig.json nor external dependencies, so I'll just put the relevant files and reports below:

Files

tsconfig.json
{
  "extends": "@kevinwolfcr/tsconfig-next",
  "compilerOptions": {
    "allowJs": true,
    "checkJs": true,
    "paths": {
      "@/*": ["./*"]
    }
  },
  "include": ["next-env.d.ts", "**/*.js", "**/*.ts", "**/*.tsx", ".next/types/**/*.ts"],
  "exclude": ["node_modules"]
}
tailwind.config.ts
import type { Config } from "tailwindcss";
import { fontFamily } from "tailwindcss/defaultTheme";
import plugin from "tailwindcss/plugin";

// ... rest of the code
app/layout.tsx
import { env } from "@/config/env";
import { cn } from "@/utils/ui";
import type { Metadata, Viewport } from "next";
import { Quattrocento } from "next/font/google";
import type { PropsWithChildren } from "react";
import "./globals.css";

// ... rest of the code

Reports

./tailwind.config.ts:2:28 lint/nursery/noUndeclaredDependencies ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  ⚠ The current dependency isn't specified in your package.json.
  
    1 │ import type { Config } from "tailwindcss";
  > 2 │ import { fontFamily } from "tailwindcss/defaultTheme";
      │                            ^^^^^^^^^^^^^^^^^^^^^^^^^^
    3 │ import plugin from "tailwindcss/plugin";
    4 │ 
  
  ℹ This could lead to errors.
  
  ℹ Add the dependency in your manifest.
  

./tailwind.config.ts:3:20 lint/nursery/noUndeclaredDependencies ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  ⚠ The current dependency isn't specified in your package.json.
  
    1 │ import type { Config } from "tailwindcss";
    2 │ import { fontFamily } from "tailwindcss/defaultTheme";
  > 3 │ import plugin from "tailwindcss/plugin";
      │                    ^^^^^^^^^^^^^^^^^^^^
    4 │ 
    5 │ export default {
  
  ℹ This could lead to errors.
  
  ℹ Add the dependency in your manifest.
  

./app/layout.tsx:1:21 lint/nursery/noUndeclaredDependencies ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  ⚠ The current dependency isn't specified in your package.json.
  
  > 1 │ import { env } from "@/config/env";
      │                     ^^^^^^^^^^^^^^
    2 │ import { cn } from "@/utils/ui";
    3 │ import type { Metadata, Viewport } from "next";
  
  ℹ This could lead to errors.
  
  ℹ Add the dependency in your manifest.
  

./app/layout.tsx:2:20 lint/nursery/noUndeclaredDependencies ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  ⚠ The current dependency isn't specified in your package.json.
  
    1 │ import { env } from "@/config/env";
  > 2 │ import { cn } from "@/utils/ui";
      │                    ^^^^^^^^^^^^
    3 │ import type { Metadata, Viewport } from "next";
    4 │ import { Quattrocento } from "next/font/google";
  
  ℹ This could lead to errors.
  
  ℹ Add the dependency in your manifest.
  

./app/layout.tsx:4:30 lint/nursery/noUndeclaredDependencies ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  ⚠ The current dependency isn't specified in your package.json.
  
    2 │ import { cn } from "@/utils/ui";
    3 │ import type { Metadata, Viewport } from "next";
  > 4 │ import { Quattrocento } from "next/font/google";
      │                              ^^^^^^^^^^^^^^^^^^
    5 │ import type { PropsWithChildren } from "react";
    6 │ import "./globals.css";
  
  ℹ This could lead to errors.
  
  ℹ Add the dependency in your manifest.
  

./components/emblem.tsx:1:20 lint/nursery/noUndeclaredDependencies ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  ⚠ The current dependency isn't specified in your package.json.
  
  > 1 │ import { cn } from "@/utils/ui";
      │                    ^^^^^^^^^^^^
    2 │ import type { SVGAttributes } from "react";
    3 │ 
  
  ℹ This could lead to errors.
  
  ℹ Add the dependency in your manifest.
  

./app/page.tsx:3:33 lint/nursery/noUndeclaredDependencies ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  ⚠ The current dependency isn't specified in your package.json.
  
    1 │ "use client";
    2 │ 
  > 3 │ import { Emblem, nations } from "@/components/emblem";
      │                                 ^^^^^^^^^^^^^^^^^^^^^
    4 │ import { useEffect, useRef, useState } from "react";
    5 │ 
  
  ℹ This could lead to errors.
  
  ℹ Add the dependency in your manifest.
  

Checked 13 files in 3ms. No fixes needed.
Found 7 warnings.

Code of Conduct

  • [X] I agree to follow Biome's Code of Conduct

kvnwolf avatar Mar 09 '24 01:03 kvnwolf

It would be great to understand what you would expect from the rule.

ematipico avatar Mar 09 '24 06:03 ematipico

Same. Biome doesn't understand path aliases(?) Would be great if it could read package.json import field, for example I have this in my Remix project:

{
  "imports": {
    "#*": "./app/*",
    "##*": "./*"
  },
}

All my imports use # basically.

And then you need to redefine them in tsconfig.json :) Btw, apparently TS 5.4.2 supports the import field so in the future this is not needed, but a lot of folks use this one only:

{
  "compilerOptions": {
    "paths": {
      "#*": ["./app/*"],
      "##*": ["./*"]
    }
  }
}

ESLint import package has this kind of syntax to define the path:

{
  settings: {
   'import/internal-regex': '^#'
  }
}

hilja avatar Mar 09 '24 11:03 hilja

It would be great to understand what you would expect from the rule.

Well what I would expect is not an error, since the path:

  1. Exist as a subpath on the next package
  2. Exist as a valid path according to my tsconfig.json paths

kvnwolf avatar Mar 10 '24 01:03 kvnwolf

@ematipico I think this could be implemented using the oxc-resolver, same as the new config loader works.

kvnwolf avatar Mar 10 '24 02:03 kvnwolf

Fixed in https://github.com/biomejs/biome/pull/2035

kvnwolf avatar Mar 11 '24 21:03 kvnwolf

@kevinwolfcr Hello I don't think this should be closed because import aliases in package.json or tsconfig.json are still not supported yet.

Sec-ant avatar Mar 12 '24 00:03 Sec-ant

Sorry @Sec-ant, I just re-opened it, I thought that since you were using oxc-resolver it would do the trick.

kvnwolf avatar Mar 12 '24 02:03 kvnwolf

No need, that's on me. I mentioned it in the PR but I didn't use that. It's just a simple fix for subpath exports only. I plan to take a further look into using oxc_resolver.

Sec-ant avatar Mar 12 '24 03:03 Sec-ant

Here is a POC to use oxc_resolver in noUndeclaredDependencies, it passes my local test:

fn run(ctx: &RuleContext<Self>) -> Self::Signals {
    let node = ctx.query();
    let token_text = node.inner_string_text()?;
    let specifier = token_text.text();
    // Fast path: read package.json
    // Ignore relative path imports
    // Ignore imports using a protocol such as `node:`, `bun:`, `jsr:`, `https:`, and so on.
    if specifier.starts_with('.') || specifier.contains(':') {
        return None;
    }
    let mut parts = specifier.split('/');
    let mut pointer = 0;
    if let Some(maybe_scope) = parts.next() {
        pointer += maybe_scope.len();
        if maybe_scope.starts_with('@') {
            // scoped package: @mui/material/Button
            // the package name is @mui/material, not @mui
            pointer += parts.next().map_or(0, |s| s.len() + 1)
        }
    }
    let package_name = &specifier[..pointer];
    if ctx.is_dependency(package_name) || ctx.is_dev_dependency(package_name) {
        return None;
    }
    // Slow path: resolve using oxc_resolver.
    let path = ctx.file_path();
    let common_resolve_options = ResolveOptions {
        tsconfig: Some(TsconfigOptions {
            // TODO: The absolute path is a demonstration.
            // We should use a "tsconfck" alternative in Rust
            // to automatically find the correct configuration file.
            config_file: PathBuf::from("/home/secant/biome/crates/biome_js_analyze/tests/specs/nursery/noUndeclaredDependencies/tsconfig.json"),
            references: TsconfigReferences::Auto,
        }),
        extensions: vec![
            ".ts".into(),
            ".js".into(),
            ".tsx".into(),
            ".jsx".into(),
            ".json".into(),
        ],
        // TODO: I think it only supports `imports` field in a `package.json` file
        // adding other file names to `description_files` doesn't work,
        // so we have to modify our test cases.
        // description_files: vec!["valid.package.json".into(), "package.json".into()]
        ..ResolveOptions::default()
    };
    // TODO: Maybe use the "type" field in package.json
    // to decide which resolver we should choose,
    // and only use both of them when we can't decide
    let esm_resolver = Resolver::new(
        common_resolve_options
            .clone()
            .with_condition_names(&["node", "import"]),
    );
    // use clone_with_options so that we can share cache between two resolvers.
    let cjs_resolver = esm_resolver.clone_with_options(
        common_resolve_options
            .clone()
            .with_condition_names(&["node", "require"]),
    );
    esm_resolver.resolve(path, specifier).map_or_else(
        |_| {
            cjs_resolver
                .resolve(path, specifier)
                .map_or(Some(()), |_| None)
        },
        |_| None,
    )
}

I don't know if we have something similar to tsconfck in rust so we can automatically resolve the corresponding tsconfig.json file path for a specific source file. Because oxc_resolver cannot automatically find the tsconfig.json file and requires us to fill its path. We can use a heuristic way to look for it for the time being.

Sec-ant avatar Mar 12 '24 07:03 Sec-ant

I don't know if we have something similar to tsconfck in rust so we can automatically resolve the corresponding tsconfig.json file path for a specific source file. Because oxc_resolver cannot automatically find the tsconfig.json file and requires us to fill its path. We can use a heuristic way to look for it for the time being.

@Sec-ant how about just assuming there will always be a tsconfig.json file on the same directory as the biome config file?

kvnwolf avatar Apr 04 '24 06:04 kvnwolf

I don't know if we have something similar to tsconfck in rust so we can automatically resolve the corresponding tsconfig.json file path for a specific source file. Because oxc_resolver cannot automatically find the tsconfig.json file and requires us to fill its path. We can use a heuristic way to look for it for the time being.

@Sec-ant how about just assuming there will always be a tsconfig.json file on the same directory as the biome config file?

We can't do that. Biome doesn't assume the presence of a configuration file; in fact, it has worked out of the box since day one. Hence, we shouldn't assume the presence of tsconfig.json too. Biome must work out of the box, always.

ematipico avatar Apr 04 '24 07:04 ematipico

Resolving tsconfig paths also impacts useImportExtensions from working properly.

Oxc requires passing tsconfig path explicitely, that could be option in short term, but breaks once you deal with monorepos and so on.

Looking into tsconfic code for resolving tsconfig.json does not look that very complicated, it just recursively goes up.

If implemented in biome where would that be? In crates/biome_js_analyze/src/services/manifest.rs?

minht11 avatar May 20 '24 16:05 minht11

@minht11 Yeah I believe so.

I am going to close this bug for now because we don't yet have the infrastructure to enhance the rule, so it can't be fixed easily. As explained, we don't read tsconfig.json at all.

If anyone wants to help with this, feel free to open a task and assign it to yourself. Of course, we are here to answer any question. If not, please bear with us while we draft a plan for it.

Cheers

ematipico avatar May 27 '24 11:05 ematipico

@ematipico can you reopen this? The problem was fixed on an earlier biome version, but after upgrading to 1.9.0 the diagnostic when importing an aliased path appeared again.

kvnwolf avatar Sep 12 '24 19:09 kvnwolf