Skip to content

Commit

Permalink
[Fix] no-cycle: fix false negative when file imports a type after i…
Browse files Browse the repository at this point in the history
…mporting a value in Flow

This fixes this situation:

`a.js`:
```js
import { foo } from './b'
```

`b.js`:
```js
// @flow
import { bar, type Baz } from './a'
```

Previously, `no-cycle` would have not reported a dependency cycle for the import in `a.js`, even
though `b.js` is importing `bar`, which is not a type import, from `a.js`. This commit fixes that.
  • Loading branch information
cherryblossom000 authored and ljharb committed May 17, 2021
1 parent 30bba6a commit 72b9c3d
Show file tree
Hide file tree
Showing 4 changed files with 15 additions and 5 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel
### Fixed
- [`no-restricted-paths`]: fix false positive matches ([#2090], thanks [@malykhinvi])
- [`no-cycle`]: ignore imports where imported file only imports types of importing file ([#2083], thanks [@cherryblossom000])
- [`no-cycle`]: fix false negative when file imports a type after importing a value in Flow ([#2083], thanks [@cherryblossom000])

### Changed
- [Docs] Add `no-relative-packages` to list of to the list of rules ([#2075], thanks [@arvigeus])
Expand Down
11 changes: 6 additions & 5 deletions src/ExportMap.js
Original file line number Diff line number Diff line change
Expand Up @@ -495,7 +495,9 @@ ExportMap.parse = function (path, content, context) {
if (n.type === 'ImportDeclaration') {
// import type { Foo } (TS and Flow)
const declarationIsType = n.importKind === 'type';
let isOnlyImportingTypes = declarationIsType;
// import './foo' or import {} from './foo' (both 0 specifiers) is a side effect and
// shouldn't be considered to be just importing types
let specifiersOnlyImportingTypes = n.specifiers.length;
const importedSpecifiers = new Set();
n.specifiers.forEach(specifier => {
if (supportedImportTypes.has(specifier.type)) {
Expand All @@ -506,11 +508,10 @@ ExportMap.parse = function (path, content, context) {
}

// import { type Foo } (Flow)
if (!declarationIsType) {
isOnlyImportingTypes = specifier.importKind === 'type';
}
specifiersOnlyImportingTypes =
specifiersOnlyImportingTypes && specifier.importKind === 'type';
});
captureDependency(n, isOnlyImportingTypes, importedSpecifiers);
captureDependency(n, declarationIsType || specifiersOnlyImportingTypes, importedSpecifiers);

const ns = n.specifiers.find(s => s.type === 'ImportNamespaceSpecifier');
if (ns) {
Expand Down
3 changes: 3 additions & 0 deletions tests/files/cycles/flow-types-some-type-imports.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
// @flow

import { foo, type BarType } from './depth-zero'
5 changes: 5 additions & 0 deletions tests/src/rules/no-cycle.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,11 @@ ruleTester.run('no-cycle', rule, {
code: 'import { foo } from "./depth-one"',
errors: [error(`Dependency cycle detected.`)],
}),
test({
code: 'import { bar } from "./flow-types-some-type-imports"',
parser: require.resolve('babel-eslint'),
errors: [error(`Dependency cycle detected.`)],
}),
test({
code: 'import { foo } from "cycles/external/depth-one"',
errors: [error(`Dependency cycle detected.`)],
Expand Down

0 comments on commit 72b9c3d

Please sign in to comment.