feat: make no-cycle ignore Flow imports (fixes #1098)

[gajus]

No description provided.

[ljharb]

LGTM

[ljharb]

sourceNode.parent seems undefined at times - many tests are failing

[gajus]

Oops. Was running tests with --grep option. Fixed.

[gajus]

@ljharb What is the fix here?

eslint <= 4 does not provide the necessary AST information to describe type import.

[ljharb]

@gajus can eslint 4 parse the types at all?

[gajus]

@gajus can eslint 4 parse the types at all?

It is the babel-eslint that enables parsing.

However, I am not familiar enough with the internals of ESLint to say what is the reason ESLint <= 4 AST visitor does not inherit properties required to describe the import kind.

All sourceNode can see is:

 Node {
  type: 'Literal',
  start: 29,
  end: 42,
  loc:
   SourceLocation {
     start: Position { line: 1, column: 29 },
     end: Position { line: 1, column: 42 } },
  range: [ 29, 42 ],
  value: './depth-one',
  raw: '"./depth-one"',
  _babelType: 'Literal' } RuleContext {
  id: 'no-cycle',
  options: [],
  settings: {},
  parserOptions:
   { sourceType: 'module',
     ecmaVersion: 6,
     ecmaFeatures: { globalReturn: false } },
  parserPath: 'babel-eslint',
  meta:
   { docs:
      { url:
         'https://github.com/benmosher/eslint-plugin-import/blob/v2.13.0/docs/rules/no-cycle.md' },
     schema: [ [Object] ] },
  eslint:
// [..]

[PinkaminaDianePie]

Whats the status of this? Without this fix eslint-plugin-import is unusable if I have Flow in my project :(

[ljharb]

The tests need to pass before it can be merged.

[PinkaminaDianePie]

Of course, but does anybody work on it? Or this PR is abandoned?

[ljharb]

If you want to help finish it, please post a link here to your branch (not a new PR) and I’ll pull in your commits :-)

[PinkaminaDianePie]

Sorry, I probably didn't get how to do it properly, but here is my branch: https://github.com/PinkaminaDianePie/eslint-plugin-import/tree/fix/no-cycle-false-positive-for-flow-imports
It works for ESLint 2-4, but I checked it manually by installing old versions - I don't really know how to check it on Travis without creating PR, so I wasn't able to check everything :(

[coveralls]

Coverage increased (+0.02%) to 97.292% when pulling 3feb54c on gajus:issue-1098 into 3b04d5f on benmosher:master.

[coveralls]

Coverage increased (+0.02%) to 97.292% when pulling 3feb54c on gajus:issue-1098 into 3b04d5f on benmosher:master.

[gajus]

Looks like the CI is failing because of unrelated tests.

[ljharb]

@gajus the travis failures are flukes; i've rerun them. the appveyor failures are unrelated, and likely in master.

[PinkaminaDianePie]

nice, i hope it will be merged finally :D

[benmosher]

not obvious to me why this is necessary but I trust @gajus so 👌🏻😄

[IhorSyerkov]

The issue appeared again in v2.17.1

[ljharb]

@IhorSyerkov please file a new issue for that.

[murrayju]

It happens in v2.17.0, but not v2.16.0. @IhorSyerkov did you make that issue? Please link it to this one.