import-js / eslint-plugin-import

ESLint plugin with rules that help validate proper imports.
MIT License
5.55k stars 1.56k forks source link

[import/no-cycle] Doesn't detect aliased imports #1554

Open christophermark opened 4 years ago

christophermark commented 4 years ago

Issue

eslint-plugin-import has an issue with aliased imports not being detected by the rule import/no-cycle.

This following example shows an import with a circular dependency that goes undetected when you change the import from a relative path, ../directory/file, to an aliased import, directoryAlias/file.

Reproducing the issue

/* main.js */

// import unused from '../exports/exporter'; // <-- Throws a no-cycle error when uncommented

import unused from 'exports/exporter'; // <-- Will not throw a no-cycle error, but it should!

export default function () {
  return;
}

Example project

To reproduce: https://github.com/christophermark/eslint-no-cycle-aliases-issue

ljharb commented 4 years ago

How is your alias defined/resolved?

christophermark commented 4 years ago

@ljharb I have the alias defined as a local NPM package in my project package.json file:

  "devDependencies": {
    "app": "file:app",
    "exports": "file:exports"
  }

And each directory aliased has a very simple package.json file to define it as a module:

./app/package.json

{
  "name": "app"
}

This alias works well with other rules such as import/no-unresolved, which is why it feels like an issue that import/no-cycle fails to find circular dependencies with the alias.

I'm not too familiar with the eslint-plugin-import internals, but is there some sort of import filtering that gets done to whitelist/blacklist imports from the dependency graph based on whether they're an internal (project-local) or external import?

ljharb commented 4 years ago

Note that file: is not an alias - npm actually copies the files using file:, so the link might not actually end up being circular.

christophermark commented 4 years ago

Interesting - that seems different from what I've observed. After installing (npm install), the directory shows up as an alias in my node_modules. Is there something I'm not seeing here?

image

ljharb commented 4 years ago

Hmm, it's possible that behavior's changed at some point in npm. What version of node? node has a --preserve-symlinks option whose default changed at some point.

christophermark commented 4 years ago

I'm using Node@12.13.0 and NPM@6.12.0 here. If you want to, you should be able to quickly reproduce locally with the example project I linked in the description 🙂

ljharb commented 4 years ago

Thanks! I see that if i change app/main to import from ../exports/exporter instead of exports/exporter, it warns.

ljharb commented 4 years ago

I've got a fix ready, and running eslint on the command line verifies that. Now I'm working on adding an "invalid" test for this.

christophermark commented 4 years ago

Thank you @ljharb! Really appreciate the fast support and turnaround on this ❤️

yunyu commented 4 years ago

@ljharb Apologies for bothering you, but would you happen to have any updates on this? Would really really like to have this fixed.

ljharb commented 4 years ago

I'm still having trouble trying to make a test case for it that works in CI.

ljharb commented 4 years ago

I've filed #1588 with what I've got so far; help getting the test to fail properly is appreciated.

jlabaj commented 3 years ago

@ljharb could anyone help you with that PR? We have a problem in the company now after upgrading to Angular 12 the Angular CLI doesn't support detecting circular dependencies out of performance issues anymore They reccomend to use linting which is not detecting the aliases as you mentioned. https://blog.ninja-squad.com/2021/05/12/angular-cli-12.0/

ljharb commented 3 years ago

@jlabaj absolutely! if you can post a link to an updated branch on #1588 (not a new PR, please!) i'd be happy to pull in your changes.

KakakuCZ commented 2 years ago

Any update?

jayarjo commented 2 years ago

What happened to this effort? Still encountering this issue.

ljharb commented 2 years ago

@jayarjo #1588 stalled, and it can be picked up again if someone wants to do the work: https://github.com/import-js/eslint-plugin-import/issues/1554#issuecomment-912131913 (not a new PR, please)

zxybte commented 10 months ago

You cau use eslint-import-resolver-alias to solve this problem. For exmaple:

import a from './src/app/goods'; => import a from '@app/goods'

// eslint-config
settings: {
    'import/resolver': {
      alias: {
        map: [['@app', './src/app']],
        extensions: ['.js', '.jsx', '.ts', '.tsx']
      },
      node: {
        extensions: ['.js', '.jsx', '.ts', '.tsx']
      }
    },
  },