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: not resolving tsconfig paths nor package subpaths #2012

Closed
1 task done
kevinwolfcr opened this issue Mar 9, 2024 · 14 comments
Closed
1 task done

Comments

@kevinwolfcr
Copy link
Contributor

kevinwolfcr commented Mar 9, 2024

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

  • I agree to follow Biome's Code of Conduct
@ematipico
Copy link
Member

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

@hilja
Copy link

hilja commented Mar 9, 2024

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': '^#'
  }
}

@kevinwolfcr
Copy link
Contributor Author

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

@kevinwolfcr
Copy link
Contributor Author

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

@kevinwolfcr
Copy link
Contributor Author

Fixed in #2035

@Sec-ant
Copy link
Member

Sec-ant commented Mar 12, 2024

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

@kevinwolfcr kevinwolfcr reopened this Mar 12, 2024
@kevinwolfcr
Copy link
Contributor Author

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

@Sec-ant
Copy link
Member

Sec-ant commented Mar 12, 2024

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
Copy link
Member

Sec-ant commented Mar 12, 2024

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.

@kevinwolfcr
Copy link
Contributor Author

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?

@ematipico
Copy link
Member

ematipico commented Apr 4, 2024

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.

@minht11
Copy link
Contributor

minht11 commented May 20, 2024

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?

@ematipico
Copy link
Member

@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 ematipico closed this as not planned Won't fix, can't repro, duplicate, stale May 27, 2024
@kevinwolfcr
Copy link
Contributor Author

@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.

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

No branches or pull requests

5 participants