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

Conflicting Eslint rules based on import order #76

Open
kateEcobb opened this issue Nov 1, 2023 · 5 comments
Open

Conflicting Eslint rules based on import order #76

kateEcobb opened this issue Nov 1, 2023 · 5 comments
Labels
documentation Improvements or additions to documentation eslint
Milestone

Comments

@kateEcobb
Copy link

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

  • Add documentation on how import order affects linting, especially in a project using turborepo, typescript, next, and internal libraries
  • Ensure that later plugin rulesets don't overwrite other styleguide rulesets

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

NikitaIT commented Nov 8, 2023

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

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

mrmckeb commented Mar 4, 2024

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?

@mrmckeb mrmckeb added documentation Improvements or additions to documentation eslint labels Mar 4, 2024
@mrmckeb mrmckeb added this to the v7 milestone Mar 4, 2024
@kateEcobb
Copy link
Author

@mrmckeb makes sense, thanks for tackling!

@sanderkooger
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation eslint
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants