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

import/no-cycle should not trigger for TypeScript type imports #1453

Closed
ValeryVS opened this issue Aug 14, 2019 · 19 comments
Closed

import/no-cycle should not trigger for TypeScript type imports #1453

ValeryVS opened this issue Aug 14, 2019 · 19 comments

Comments

@ValeryVS
Copy link

Repro

{
  parser: '@typescript-eslint/parser',
  parserOptions: {
    project: './tsconfig.json',
  },
  plugins: [
    '@typescript-eslint',
    'react-hooks',
  ],
  extends: [
    'airbnb',
    'plugin:import/typescript',
    'plugin:@typescript-eslint/eslint-recommended',
    'plugin:@typescript-eslint/recommended',
  ],
  // ...
}
// store.ts
import orders from 'entities/orders/reducers';

const rootReducer = combineReducers({
  orders,
});

export type AppState = ReturnType<typeof rootReducer>
// entities/orders/reducers.ts
import {SetOrdersAction} from './actions';

export default createReducer(initialState, {
 ...
})
// entities/orders/actions.ts
import {ThunkAction} from 'redux-thunk';

import {AppState} from 'store';

export const pollOrders = (): ThunkAction<void, AppState, void, SetOrdersAction> => (
   ...
);

Actual Result

There is the store (imports reducers), reducers (imports actions), and actions (imports store type) in application.
import/no-cycle rule alerts about cycle dependency, but only type information are cycled and it will be skipped at runtime

Related
#1343
typescript-eslint/typescript-eslint#843

@ljharb
Copy link
Member

ljharb commented Aug 14, 2019

Flow has import type; TS does not - I’m not sure how we could statically know it’s a type on the import side, especially with d.ts files being a thing.

@jquense
Copy link
Contributor

jquense commented Aug 15, 2019

The tsc and babel both make best guesses about whether an import is "type only" and remove the import if it is. There is likely a high reliability of detecting it given they feel comfortable enough to remove imports. Unclear whether it's analyzable in a performant way tho

@ljharb
Copy link
Member

ljharb commented Aug 15, 2019

If the ast node has that info on it then it seems possible to ignore it - a PR would be great.

@jquense
Copy link
Contributor

jquense commented Aug 15, 2019

My understanding is it's not the import nodes that have it, there is some amount of scope tracking to test it. I'm not qualified to write this but if anyone is interested in where the babel logic is: https://github.com/babel/babel/blob/master/packages/babel-plugin-transform-typescript/src/index.js#L382

@grabofus
Copy link

grabofus commented Feb 26, 2020

Flow has import type; TS does not - I’m not sure how we could statically know it’s a type on the import side, especially with d.ts files being a thing.

As of TypeScript 3.8, import type is available in TS too, it might help with the implementation.

@sunopar
Copy link

sunopar commented Feb 28, 2020

same issue, has any solution?

@ljharb
Copy link
Member

ljharb commented Feb 28, 2020

nope, nobody's sent a PR afaict.

@MetabaronCZ
Copy link

Hi, using import type { x } from 'y'; solved problem for me.

@kaatt
Copy link

kaatt commented Apr 13, 2020

nope, nobody's sent a PR afaict.

do you have any tips/directions for someone to begin work on a PR?

@ljharb
Copy link
Member

ljharb commented Apr 13, 2020

For eslint plugins, I'd start by making as many test cases as you can manage, and then you can start debugging what node types you'd need to work with to fix them.

@SweVictor
Copy link

This would be a great feature! Would like to add that typescript interfaces should also be safe to exclude from this rule since they will be removed when transpiling to js.

As a side not, the rule also seems to trigger on a file-by-file basis rather than on an import-by-import basis. In our code-base we have a "Models" directory with data models, often linking to each other. An index.ts includes all models and exports them together.

import { Model1, Model2 } from "."
is much easier and more readable than

import { Model1 } from "./Model1"
import { Model2 } from "./Model2"

but the first code style will trigger lots of no-cycle errors most of which can be removed by using the second more verbose syntax.

@Sytten
Copy link

Sytten commented Jul 17, 2020

Just bumping since this is fairly annoying.

@MetabaronCZ
Copy link

I can confirm, that using syntax import type { x } from 'y' works without problem.

@ljharb
Copy link
Member

ljharb commented Jul 17, 2020

@Sytten PRs are welcome.

@nickzelei
Copy link

With Typescript's new import type syntax, this error goes away.

@hakan-bilgin
Copy link

I have some files in my codebase which exports types and also enums. Once I use import type { x } from 'y' syntax, TypeScript complains and raises following error; 'MyEnum' cannot be used as a value because it was imported using 'import type'.ts(1361). So, I had to exclude this rule from my rules list. If I import my enum as before, this time I get cycle dependency error. So, import type { x } from 'y' is not a solution for enums, I had to exclude this rule from my rule set.

@RebeccaStevens
Copy link

import type is a solution for types. Enums are not types...

@LucaColonnello
Copy link

@RebeccaStevens In case of a circular dependency, as enums are compiled to values in TS, it's fair that the rule applies.
You might have issues in situations where you're expecting a value to be defined but it's not yet due to the circular dependency issue, which can cause unpredictable problems depending on where the value is used, which is the point of the rule.

So enums should be treated with the same regards as any other imported value :)

@JounQin
Copy link
Collaborator

JounQin commented Jul 19, 2022

close in favor of above

@JounQin JounQin closed this as not planned Won't fix, can't repro, duplicate, stale Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests