vercel / style-guide

Vercel's engineering style guide
Mozilla Public License 2.0
1.18k stars 29 forks source link

Enabling import/no-extraneous-dependencies inside rules/import.js introduces bug with path aliases #71

Closed JaViLuMa closed 9 months ago

JaViLuMa commented 9 months ago

Hello!

So from the title above there is a bug (maybe not) introduced when this line:

'import/no-extraneous-dependencies': [
  'error',
    { includeInternal: true, includeTypes: true }, // <----- this one
],

was enabled in the newer version of the style guide.

The bug is introduced when it comes to Typescript's path aliasing, which always gives me this error: image 'level5' should be listed in the project's dependencies. Run 'npm i -S level5' to add

Where level5 is literally the project I am currently working on:

{
  "name": "level5",
  "version": "0.1.0",
  "private": true,
  "scripts": {
    "build": "next build",
    "dev": "next dev",
    "lint": "next lint",
    "start": "next start"
  },
  "dependencies": {
    "@types/node": "20.6.3",
    "@types/react": "18.2.22",
    "@types/react-dom": "18.2.7",
    ...
  },
  "devDependencies": {
    "@next/eslint-plugin-next": "^13.5.2",
    "@typescript-eslint/eslint-plugin": "^6.7.2",
    "@typescript-eslint/parser": "^6.7.2",
    "@vercel/style-guide": "^5.0.1",
    "eslint-import-resolver-typescript": "^3.6.0",
    "eslint-plugin-import": "^2.28.1",
  }
}

I have two projects that use this style guide and they both have the same identical setup when it comes to tsconfig.json and .eslintrc.json.

The project where the issue doesn't appear uses the "@vercel/style-guide": "^4.0.2" while this level5 one uses "@vercel/style-guide": "^5.0.1".

This is my .eslintrrc.js file:

const { resolve } = require('path');

const project = resolve(__dirname, 'tsconfig.json');

module.exports = {
  root: true,
  extends: [
    require.resolve('@vercel/style-guide/eslint/browser'),
    require.resolve('@vercel/style-guide/eslint/react'),
    require.resolve('@vercel/style-guide/eslint/next'),
    require.resolve('@vercel/style-guide/eslint/typescript'),
  ],
  parser: '@typescript-eslint/parser',
  parserOptions: {
    project,
  },
  ignorePatterns: ['.eslintrc.js', 'next.config.js', 'tailwind.config.js', '.prettierrc.js'],
  settings: {
    'import/resolver': {
      typescript: {
        project,
      },
    },
    'jsx-a11y': {
      components: {
        Article: 'article',
        Button: 'button',
        Image: 'img',
        Input: 'input',
        Link: 'a',
        Video: 'video',
      },
    },
  },
  rules: {
    '@typescript-eslint/consistent-type-definitions': 'off',
    'unicorn/filename-case': [
      'error',
      {
        cases: {
          camelCase: true,
          pascalCase: true,
          kebabCase: true,
        },
      },
    ],
  },
};

And my tsconfig.json file:

{
  "compilerOptions": {
    "target": "es5",
    "lib": ["dom", "dom.iterable", "esnext"],
    "allowJs": true,
    "skipLibCheck": true,
    "strict": true,
    "forceConsistentCasingInFileNames": true,
    "noEmit": true,
    "esModuleInterop": true,
    "module": "esnext",
    "moduleResolution": "bundler",
    "resolveJsonModule": true,
    "isolatedModules": true,
    "jsx": "preserve",
    "incremental": true,
    "plugins": [
      {
        "name": "next"
      }
    ],
    "paths": {
      "@/*": ["./src/*"]
    }
  },
  "include": ["next-env.d.ts", "**/*.ts", "**/*.tsx", ".next/types/**/*.ts"],
  "exclude": ["node_modules"],
  "extends": "@vercel/style-guide/typescript"
}
mrmckeb commented 9 months ago

Thanks for reporting this. Unfortunately most of the projects we've rolled this out to internally don't use aliases, so we hadn't yet hit this issue.

I've reproduced and also found the upstream issue: https://github.com/import-js/eslint-plugin-import/issues/2617

I'll create a PR to disable the offending change now!

vercel-release-bot commented 9 months ago

:tada: This issue has been resolved in version 5.0.2-canary.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

mrmckeb commented 9 months ago

Can you test the canary release (above) and let us know if that resolves your issue @JaViLuMa?

There's nothing else in that release :)

JaViLuMa commented 9 months ago

I definitely can! Give me few minutes!

But since the issue is only introduced in Typescript, maybe we can disable that rule or remove the includeInternal inside the Typescript's import rules so it stays enabled for JS?

JaViLuMa commented 9 months ago

So I tested version 5.0.2-canary.1 and it definitely works now 😄

ShaneYu commented 8 months ago

Was having this very same issue and 5.0.2-canary.1 resolved this right away. Thank you.

vercel-release-bot commented 7 months ago

:tada: This issue has been resolved in version 5.1.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: