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

fix: ensure absolute paths are handled correctly with --file option in next lint command for lint-staged compatibility #69220

Open
wants to merge 6 commits into
base: canary
Choose a base branch
from

Conversation

lumirlumir
Copy link

Hello😊 Thanks for having attention to this issue.

What?

Fixed a bug.

The next lint command's --file option was intended to allow users to lint specific files directly by specifying their paths. However, it only worked with relative paths and did not handle absolute paths correctly. This limitation hindered the flexibility and usability of the linting process.

Furthermore, lint-staged returns absolute paths when running lint commands on staged files. Without proper handling of these absolute paths, linting was not performed correctly, leading to issues in automated linting workflows.

Why?

Environment(results by next info)

Operating System:
  Platform: win32
  Arch: x64
  Version: Windows 11 Home
  Available memory (MB): 16058
  Available CPU cores: 8
Binaries:
  Node: 20.12.2
  npm: N/A
  Yarn: N/A
  pnpm: N/A
Relevant Packages:
  next: 14.2.6 // Latest available version is detected (14.2.6).
  eslint-config-next: 14.2.6
  react: 18.3.1
  react-dom: 18.3.1
  typescript: 5.5.4
Next.js Config:
  output: N/A

Steps to Reproduce

  1. Make a js or jsx file like below. this makes ESLint error named no-unused-vars

    /* src/app/test.jsx */
    
    const a = 1;
  2. Run eslint and next lint to the same file.

    See screenshot below.

    eslint command works correctly with absolute path. but, next lint command doesn't work correctly with absolute path.

    스크린샷 2024-08-23 173447

Code Details

/* packages/next/src/cli/next-lint.ts */

// ...

const pathsToLint = (
    filesToLint.length ? filesToLint : ESLINT_DEFAULT_DIRS
  ).reduce((res: string[], d: string) => {
    const currDir = join(baseDir, d)

    if (!existsSync(currDir)) {
      return res
    }

    res.push(currDir)
    return res
  }, [])

// ...

pathsToLint

  • pathsToLint gets an empty array [] when filesToLint array is [ 'D:\\Cloud_Git\\web-blog\\src\\app\\test.jsx' ] passed by --file option from CLI.

The reduce Function

  • For each element (d) in filesToLint, it combines it with baseDir to create currDir.
  • It checks if this path exists using existsSync(currDir).
  • If the path does not exist, it does not add it to the resulting array (res), and if it does exist, it adds it.

Cause of the Issue

  • join(baseDir, d):

    • The join function is used to combine file paths and directory paths.
    • If baseDir is D:\Cloud_Git\web-blog, then currDir becomes D:\Cloud_Git\web-blog\D:\Cloud_Git\web-blog\src\app\test.jsx, creating an incorrect path.
  • existsSync(currDir):

    • With the incorrect path (D:\Cloud_Git\web-blog\D:\Cloud_Git\web-blog\src\app\test.jsx), existsSync determines that this path does not exist, so it does not add it to the res array.
    • As a result, pathsToLint ends up being an empty array [].

How? (Solution)

The code should be modified to use the file path as-is, without combining it with baseDir. If a file path is provided, it should be directly included in pathsToLint.

  • Updated Path Handling: Modified the code that processes the --file option to properly handle absolute paths. This involved ensuring that absolute paths are correctly interpreted and used by the linting command.
  • Enhanced Compatibility with lint-staged: Adjusted the linting logic to handle absolute paths returned by lint-staged, ensuring that linting works as expected in automated workflows.

The code below is the built code of the modified version.

/* node_modules/next/dist/cli/next-lint.js */

// ...

const pathsToLint = (filesToLint.length ? filesToLint : _constants.ESLINT_DEFAULT_DIRS).reduce((res, d)=>{
    const currDir = (0, _fs.existsSync)(d) ? d : (0, _path.join)(baseDir, d);

    if (!(0, _fs.existsSync)(currDir)) {
        return res;
    }
    res.push(currDir);
    return res;
}, []);

// ...

Closes: I tried to find but No related Issues.
Fixes: I tried to find but No related Issues.

@ijjk
Copy link
Member

ijjk commented Aug 23, 2024

Allow CI Workflow Run

  • approve CI run for commit: 40130e9

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants