Add type import support to import/order
gajus opened this issue · 17 comments
Example:
import React from 'react';
import reactCssModules from 'react-css-modules';
import styles from './static/styles.css';
import type {
TopicType
} from '../../types';Refer to https://flowtype.org/blog/2015/02/18/Import-Types.html
+1. I import my types with modules path (not relative), as I use webpack alias. I like to import types after all ( the module + relative import) non type imports like.
import React from 'react';
import A from '../';
import type { TopicType } from 'app/types';
I think this makes sense, need @jfmengels to manage the resolution, though.
Not sure if this is a new flowtype classification analogous to external, etc., or if it's another level of ordering. Needs some more discussion.
We could make that a new importType value (like external), which sounds good to me. The "problem" is stylistic:
if some people want to have their imports glued together without empty lines, then the type imports, with both groups separated by a line.
import path from 'path';
import a from './a';
import type Foo from 'foo';Or if some people want to have their types next to the imported package
import path from 'path';
import foo from 'foo';
import type Foo from 'foo';
import a from './a';That said, you'd get the same problem if you wanted to order things by resource type (js, json, css, types, ...), which is not possible at the moment.
Also, maybe you'd want to order your type imports by the import type of the module.
import type A from './a'; // this import should be after import of 'foo'
import type Foo from 'foo';Any thoughts?
yeah, that's where I almost feel like some sort of tiered ordering is needed, but I don't know how it would be specified. the existing types are a function of the import path, but this new flow type is a function of the import itself, so they're orthogonal classifications.
which is how you end up with the options you've described.
maybe just ignore type imports. I personally prefer,
import path from 'path';
import a from './a';
import type Foo from 'foo';
as types are just compile time thing, and get a good sense of all the imports if I group them. If this plugin ignores type imports, then user can either separate it at the end as below, or together with the module import.
In Flow v0.38.0, there is a new feature that allows you to import both values and types in a single import statement.
New shorthand for importing types in the same declaration that imports values: import {someValue, type someType, typeof someOtherValue} from 'foo'
I haven't used it yet, but that sounds better and I feel like we should encourage the use of that syntax rather than one separating the import of types and the import of values.
This way, the order rule wouldn't need to be adapted, which sounds better to me. With the proposed solution, you'd have first the values imports, then the types imports, but the types wouldn't be sorted according to where they are from, which sounds pretty weird to me now.
Maybe eslint-plugin-flow could implement a rule enforcing the use of that syntax @gajus ?
What do you think?
(cc @avaly who already started working on this)
A problem I frequently run into, as well, is importing types from relative vs absolute paths.
// @flow
import fs from 'fs';
import Foo from './FooClass';
import type { SomeGlobalType } from 'my-global-types-pkg';
import type { LocalModuleType } from './types';It makes sense to me to group the types together as it makes reading the code more natural. Probably related with https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/first.md as well. Any thoughts?
@jfmengels , but what if i have to import the default module and a type?
/* @flow */
import type { awesomeType } from './awesomeModule.js'
import awesomeDefaultModule from './awesomeModule.js'@emil14 you can do:
import awesomeDefaultModule, { type awesomeType } from './awesomeModule.js'@christophehurpeau wow thank you, I'll try it
Any news on this? I also feel that @jribeiro's suggestion feels the most natural. I'd prefer not mix type imports with other stuff. Regardless, if the gods of js say otherwise, I'd adapt.
Also interested in this rule.
This is coming to TypeScript in 3.8 as well: https://devblogs.microsoft.com/typescript/announcing-typescript-3-8-rc/#type-only-imports-exports
I've written a patch that adds type import support and handles alphabetisation when using separate type imports for the same module.
Example imports
import storage from '@react-native-async-storage/async-storage';
import {
offlineActionTypes,
checkInternetConnection
} from 'react-native-offline';
import { composeWithDevTools } from 'redux-devtools-extension';
import { createMigrate, persistStore, persistReducer } from 'redux-persist';
import thunk from 'redux-thunk';
import { createStore, applyMiddleware } from 'redux';
import { migrations, version, key } from 'app/store/migrations';
import rootReducer from 'app/store/reducers';
import APIConnector from 'app/utils/api-connector';
import { loadDeviceInfo } from 'app/utils/device-info';
import type { StateType } from 'app/store/reducers';
import type { Persistor } from 'redux-persist/src/types';
import type { Store, DispatchAPI } from 'redux';.eslintrc
{
"rules": {
"import/order": [
"error",
{
"groups": ["external", "internal", "types"],
"pathGroups": [
{
"pattern": "app/**",
"group": "internal"
}
],
"pathGroupsExcludedImportTypes": ["types"],
"newlines-between": "always",
"alphabetize": { "order": "asc" }
}
]
}
}eslint-plugin-import+2.22.1.patch
diff --git a/node_modules/eslint-plugin-import/lib/rules/order.js b/node_modules/eslint-plugin-import/lib/rules/order.js
index d3cd442..733af22 100644
--- a/node_modules/eslint-plugin-import/lib/rules/order.js
+++ b/node_modules/eslint-plugin-import/lib/rules/order.js
@@ -265,7 +265,7 @@ function mutateRanksToAlphabetize(imported, alphabetizeOptions) {
if (!Array.isArray(acc[importedItem.rank])) {
acc[importedItem.rank] = [];
}
- acc[importedItem.rank].push(importedItem.value);
+ acc[importedItem.rank].push(`${importedItem.value}-${importedItem.node.importKind}`);
return acc;
}, {});
@@ -290,7 +290,7 @@ function mutateRanksToAlphabetize(imported, alphabetizeOptions) {
// mutate the original group-rank with alphabetized-rank
imported.forEach(function (importedItem) {
- importedItem.rank = alphabetizedRanks[importedItem.value];
+ importedItem.rank = alphabetizedRanks[`${importedItem.value}-${importedItem.node.importKind}`];
});
}
@@ -310,6 +310,8 @@ function computeRank(context, ranks, importEntry, excludedImportTypes) {
let rank;
if (importEntry.type === 'import:object') {
impType = 'object';
+ } else if (importEntry.type === 'import' && importEntry.node.importKind === 'type') {
+ impType = 'types';
} else {
impType = (0, _importType2.default)(importEntry.value, context);
}
@@ -338,7 +340,7 @@ function isInVariableDeclarator(node) {
node.type === 'VariableDeclarator' || isInVariableDeclarator(node.parent));
}
-const types = ['builtin', 'external', 'internal', 'unknown', 'parent', 'sibling', 'index', 'object'];
+const types = ['builtin', 'external', 'internal', 'unknown', 'parent', 'sibling', 'index', 'object', 'types'];
// Creates an object with type-rank pairs.
// Example: { index: 0, sibling: 1, parent: 1, external: 1, builtin: 2, internal: 2 }
@grit96 would you mind making a PR with that patch, and some test cases?
Is this a breaking change for TypeScript users that are importing types together with the primary import?
eg
import { Bar } from 'bar';
import type { BarType } from 'bar';
import { Foo } from 'foo';I don't believe TypeScript supports importing both types and non-types in the same import line like flow allows.
eg The following is not supported:
import { Bar, type BarType } from 'bar';Is there a way to add an option to retain the previous behaviour?