vercel / style-guide

Vercel's engineering style guide
Mozilla Public License 2.0
1.25k stars 33 forks source link

Conflicting Eslint rules based on import order #76

Open kateEcobb opened 11 months ago

kateEcobb commented 11 months ago

I followed the with-tailwind example, which uses the new eslint rules from vercel/styleguide. I was having issues getting eslint to recognize imports from the packages directory, even though typescript could parse it fine.

From the example, the next.js eslint file lists the plugins in this order:

module.exports = {
  extends: [
    "@vercel/style-guide/eslint/node",
    "@vercel/style-guide/eslint/typescript",
    "@vercel/style-guide/eslint/browser",
    "@vercel/style-guide/eslint/react",
    "@vercel/style-guide/eslint/next",
    "eslint-config-turbo",
  ].map(require.resolve),

Expected behavior

Imports from UI directory are able to be resolved correctly

Actual behavior

This error, which fails when linting and within my IDE:

Workaround solution

I reordered the style guide rules so that the typescript rules come last:

module.exports = {
    extends: [
        "@vercel/style-guide/eslint/node",
        "@vercel/style-guide/eslint/browser",
        "@vercel/style-guide/eslint/react",
        "@vercel/style-guide/eslint/next",
        "eslint-config-turbo",
        "@vercel/style-guide/eslint/typescript",
    ].map(require.resolve),

I only found this when I was digging through the repo and found this configuration:

/**
 * These are enabled by `import/recommended`, but are better handled by
 * TypeScript and @typescript-eslint.
 */
const disabledRules = {
  'import/default': 'off',
  'import/export': 'off',
  'import/namespace': 'off',
  'import/no-unresolved': 'off',
};

module.exports = {
  rules: {
    ...disabledRules,
  },
};

Proposed solution

Operating system

Apple M1 pro

Setup / packages

Next 13.5 Postcss + tailwind turborepo typescript custom eslint

Project layout

├── apps
│   ├── web -- Nextjs app
│   ├── design -- storybook app
│   ├── legacy-web -- old create react app bundle, will eventually be deprecated
├── packages
│   ├── eslint-config-custom (copied from `with-tailwind` example)
│   ├── tsconfig (copied from `with-tailwind` example)
│   ├── tailwind-config (copied from `with-tailwind` example)
│   ├── ui -- basic UI directory, very similar to the UI directories in the examples except with two entry points (server vs client)
├── package.json
├── turbo.json
├── tsconfig.json (base tsconfig applied)
└── .gitignore, etc
NikitaIT commented 10 months ago

package.json

"types": "./dist/index.d.ts"

Build output:

ESM dist/card.mjs           50.00 B
ESM dist/chunk-MFXFT5EY.mjs 730.00 B
ESM dist/index.css          5.67 KB
ESM dist/index.mjs          50.00 B
ESM ⚡️ Build success in 273ms
DTS ⚡️ Build success in 582ms
DTS dist/card.d.mts  188.00 B
DTS dist/index.d.mts 51.00 B

So, valid package.json should be like:

"types": "./dist/index.d.mts"

Or better:

tsup.config.ts

format: [
"esm", 
+"cjs"
],
kateEcobb commented 10 months ago

That still doesn't work, as I export two types files from my ui directory, one for a client package (meant to be used on the client only with the use client directive), and another than can be used on either server or client. My package.json in the UI directory is as follows:

{
  "name": "ui",
  "version": "0.0.0",
  "license": "MIT",
  "sideEffects": [
    "**/*.css"
  ],
  "exports": {
    ".": "./dist",
    "./client": "./dist/client",
    "./styles.css": "./dist/index.css"
  },
  "typesVersions": {
    "*": {
      "*": [
        "src/*",
        "src/index",
        "src/client"
      ]
    }
  },
  "scripts": {
    "build": "tsup",
    "check-types": "tsc --noEmit",
    "clean": "rm -rf dist && rm -rf node_modules && rm -rf .turbo",
    "dev": "tsup --watch",
    "lint": "eslint \"**/*.ts*\"",
    "lint:fix": "eslint \"**/*.ts*\" --fix"
  },
  "dependencies": {
    "@mapbox/search-js-react": "^1.0.0-beta.18",
    "lodash": "^4.17.21",
    "react-hook-form": "^7.45.4",
    "uuid": "^9.0.1"
  },
  "devDependencies": {
    "@mapbox/search-js-core": "^1.0.0-beta.18",
    "@types/react": "^18.2.33",
    "@types/uuid": "^9.0.7",
    "autoprefixer": "^10.4.14",
    "eslint-config-custom": "*",
    "postcss": "^8.4.4",
    "react": "^18.2.0",
    "tailwind-config": "*",
    "tailwindcss": "^3.3.3",
    "tsconfig": "*",
    "tsup": "^7.2.0",
    "typescript": "^5.0.4"
  },
  "peerDependencies": {
    "react": "^18.2.0"
  },
  "publishConfig": {
    "typesVersions": {
      "*": {
        "*": [
          "dist/*.d.mts",
          "dist/*.d.ts"
        ]
      }
    }
  }
}

I still have to place the typescript eslint rule below browser + react (which isn't how it is in the with-tailwind example). My extends block in next.js is:

 extends: [
        "@vercel/style-guide/eslint/node",
        "@vercel/style-guide/eslint/browser",
        "@vercel/style-guide/eslint/react",
        "@vercel/style-guide/eslint/typescript",
        "@vercel/style-guide/eslint/next",
        "eslint-config-turbo",
    ].map(require.resolve),

My tsup config within the UI directory:

import type { Options } from 'tsup';
import { defineConfig } from 'tsup';

export default defineConfig((options: Options) => ({
  treeshake: true,
  splitting: true,
  entry: ['src/index.tsx', 'src/client.tsx'],
  format: ['esm'],
  dts: true,
  minify: true,
  clean: true,
  external: ['react'],
  ...options,
}));

I think adding some documentation about extends order would suffice?

mrmckeb commented 6 months ago

This might need a docs overhaul. I think we are going to do a rewrite soon for flat configs anyway... so might leave it until then?

kateEcobb commented 6 months ago

@mrmckeb makes sense, thanks for tackling!

sanderkooger commented 2 months ago

@mrmckeb if you are going to do a rewrite. Might I also ask to make a modular turbo repo example. Where the configs are a package in the package folder. And configs are nested.

Like base ---> node --> react --> nextjs for example. This would allow us to use the same set of base rules everywhere in our repos.

Thanks, looking forward to Flat configs! But Next.js does not support that yet, right ?