[New] no-duplicates: add combineTypeImports option to de-duplicate flow type imports
#2229
Conversation
…e flow type imports
Codecov Report
@@ Coverage Diff @@
## main #2229 +/- ##
==========================================
+ Coverage 84.02% 84.03% +0.01%
==========================================
Files 94 94
Lines 3017 3020 +3
Branches 898 900 +2
==========================================
+ Hits 2535 2538 +3
Misses 482 482
Continue to review full report at Codecov.
|
| @@ -61,6 +61,25 @@ import SomeDefaultClass from './mod?minify' | |||
| import * from './mod.js?minify' | |||
| ``` | |||
|
|
|||
| ### combineTypeImports | |||
|
|
|||
| By default, this rule ignores flow & typescript type imports. However you can use the option `combineTypeImports` to combine type imports with regular imports. | |||
There was a problem hiding this comment.
can you explain why this is a good thing, or how it would apply to TS? In TS, it's preferred to use import type for all type imports, and import only for value imports.
Does Flow not have a separate import type?
There was a problem hiding this comment.
can you explain why this is a good thing, or how it would apply to TS? In TS, it's preferred to use
import typefor all type imports, andimportonly for value imports.
TypeScript allows to not use import type for type imports. typescript-eslint has a rule to specify the syntax: typescript-eslint/consistent-type-imports. As you are saying it's not the preferred way, so it shouldn't be the default, but it is something users might want to set.
The main reason we would like to combine type and regular imports in our flow code is to make it easy for developers to see what the dependencies on a certain file. Some of our files have both regular and type exports so when they are imported, it's confusing to not see them combined:
Before
// @flow
import { type I18nContext } from 'app/packages/i18n/useI18n';
// Other imports
import useI18n, { I18nContextProvider } from 'app/packages/i18n/useI18n';
After
// @flow
import useI18n, { I18nContextProvider, type I18nContext } from 'app/packages/i18n/useI18n';
Does Flow not have a separate import type?
Flow supports both import type Foo and import { type Foo } syntax.
There was a problem hiding this comment.
I can’t imagine why you’d prefer the combined form over the separate import vs import type statements, but at least i understand what the option would do.
Since they can’t be combined in TS, this option would only ever be useful for Flow. I’m not sure supporting only flow is worth the complexity at this time due to the current state of Flow usage in the ecosystem.
There was a problem hiding this comment.
@ljharb understood, closing this PR for now.
There was a problem hiding this comment.
Looks like in TS 4.5+, they can be combined. I'll reopen.
|
Closing since I no longer plan to work on this. Anyone should feel free to open a new PR and can use this PR as inspiration. |
|
That’s fine, but let’s leave it open so this PR can be updated later :-) |
Description
#334 always duplicated flow type imports. In this diff we add the
combineTypeImportsoption to be able to de-duplicate flow type imports.TypeScript support can be added later, for now we only support combining regular & flow type imports.
Fixes: #909
Related PRs / Issues
no-duplicatesforce type & normal imports to be de-duplicated #909: Request: option to let no-duplicates force type & normal imports to be de-duplicated