-
Notifications
You must be signed in to change notification settings - Fork 25
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
Support explicit type sorting #44
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
126 changes: 126 additions & 0 deletions
126
src/utils/__tests__/explode-type-and-value-specifiers.spec.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,126 @@ | ||
import { explodeTypeAndValueSpecifiers } from '../explode-type-and-value-specifiers'; | ||
import { getCodeFromAst } from '../get-code-from-ast'; | ||
import { getImportNodes } from '../get-import-nodes'; | ||
|
||
test('it should return a default value import unchanged', () => { | ||
const code = `import Default from './source';`; | ||
const importNodes = getImportNodes(code); | ||
const explodedNodes = explodeTypeAndValueSpecifiers(importNodes); | ||
const formatted = getCodeFromAst({ | ||
nodesToOutput: explodedNodes, | ||
originalCode: code, | ||
directives: [], | ||
}); | ||
expect(formatted).toEqual(`import Default from './source';`); | ||
}); | ||
|
||
test('it should return a default value and namespace import unchanged', () => { | ||
const code = `import Default, * as Namespace from './source';`; | ||
const importNodes = getImportNodes(code); | ||
const explodedNodes = explodeTypeAndValueSpecifiers(importNodes); | ||
const formatted = getCodeFromAst({ | ||
nodesToOutput: explodedNodes, | ||
originalCode: code, | ||
directives: [], | ||
}); | ||
expect(formatted).toEqual( | ||
`import Default, * as Namespace from './source';`, | ||
); | ||
}); | ||
|
||
test('it should return default and namespaced value imports unchanged', () => { | ||
const code = `import Default, { named } from './source';`; | ||
const importNodes = getImportNodes(code); | ||
const explodedNodes = explodeTypeAndValueSpecifiers(importNodes); | ||
const formatted = getCodeFromAst({ | ||
nodesToOutput: explodedNodes, | ||
originalCode: code, | ||
directives: [], | ||
}); | ||
expect(formatted).toEqual(`import Default, { named } from './source';`); | ||
}); | ||
|
||
test('it should return default type imports unchanged', () => { | ||
const code = `import type DefaultType from './source';`; | ||
const importNodes = getImportNodes(code, { plugins: ['typescript'] }); | ||
const explodedNodes = explodeTypeAndValueSpecifiers(importNodes); | ||
const formatted = getCodeFromAst({ | ||
nodesToOutput: explodedNodes, | ||
originalCode: code, | ||
directives: [], | ||
}); | ||
expect(formatted).toEqual(`import type DefaultType from './source';`); | ||
}); | ||
|
||
test('it should return namespace type imports unchanged', () => { | ||
const code = `import type * as NamespaceType from './source';`; | ||
const importNodes = getImportNodes(code, { plugins: ['typescript'] }); | ||
const explodedNodes = explodeTypeAndValueSpecifiers(importNodes); | ||
const formatted = getCodeFromAst({ | ||
nodesToOutput: explodedNodes, | ||
originalCode: code, | ||
directives: [], | ||
}); | ||
expect(formatted).toEqual( | ||
`import type * as NamespaceType from './source';`, | ||
); | ||
}); | ||
|
||
test('it should return named type imports unchanged', () => { | ||
const code = `import type { NamedType1, NamedType2 } from './source';`; | ||
const importNodes = getImportNodes(code, { plugins: ['typescript'] }); | ||
const explodedNodes = explodeTypeAndValueSpecifiers(importNodes); | ||
const formatted = getCodeFromAst({ | ||
nodesToOutput: explodedNodes, | ||
originalCode: code, | ||
directives: [], | ||
}); | ||
expect(formatted).toEqual( | ||
`import type { NamedType1, NamedType2 } from './source';`, | ||
); | ||
}); | ||
|
||
test('it should separate named type and value imports', () => { | ||
const code = `import { named, type NamedType } from './source';`; | ||
const importNodes = getImportNodes(code, { plugins: ['typescript'] }); | ||
const explodedNodes = explodeTypeAndValueSpecifiers(importNodes); | ||
const formatted = getCodeFromAst({ | ||
nodesToOutput: explodedNodes, | ||
originalCode: code, | ||
directives: [], | ||
}); | ||
expect(formatted).toEqual( | ||
`import { named } from './source'; | ||
import type { NamedType } from './source';`, | ||
); | ||
}); | ||
|
||
test('it should separate named type and default value imports', () => { | ||
const code = `import Default, { type NamedType } from './source';`; | ||
const importNodes = getImportNodes(code, { plugins: ['typescript'] }); | ||
const explodedNodes = explodeTypeAndValueSpecifiers(importNodes); | ||
const formatted = getCodeFromAst({ | ||
nodesToOutput: explodedNodes, | ||
originalCode: code, | ||
directives: [], | ||
}); | ||
expect(formatted).toEqual( | ||
`import Default from './source'; | ||
import type { NamedType } from './source';`, | ||
); | ||
}); | ||
|
||
test('it should separate named type and default and named value imports', () => { | ||
const code = `import Default, { named, type NamedType } from './source';`; | ||
const importNodes = getImportNodes(code, { plugins: ['typescript'] }); | ||
const explodedNodes = explodeTypeAndValueSpecifiers(importNodes); | ||
const formatted = getCodeFromAst({ | ||
nodesToOutput: explodedNodes, | ||
originalCode: code, | ||
directives: [], | ||
}); | ||
expect(formatted).toEqual( | ||
`import Default, { named } from './source'; | ||
import type { NamedType } from './source';`, | ||
); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
import { importDeclaration, type ImportSpecifier } from '@babel/types'; | ||
|
||
import { ExplodeTypeAndValueSpecifiers } from '../types'; | ||
|
||
/** | ||
* Breaks apart import declarations containing mixed type and value imports into separate declarations. | ||
* | ||
* e.g. | ||
* | ||
* ```diff | ||
* - import foo, { bar, type Baz } from './source'; | ||
* + import foo, { bar } from './source'; | ||
* + import type { Baz } from './source'; | ||
* ``` | ||
*/ | ||
export const explodeTypeAndValueSpecifiers: ExplodeTypeAndValueSpecifiers = ( | ||
nodes, | ||
) => { | ||
const explodedNodes = []; | ||
|
||
for (const node of nodes) { | ||
// We don't need to explode type imports, they won't mix type and value | ||
if (node.importKind === 'type') { | ||
explodedNodes.push(node); | ||
continue; | ||
} | ||
|
||
// Nothing to do if there's only one specifier | ||
if (node.specifiers.length <= 1) { | ||
explodedNodes.push(node); | ||
continue; | ||
} | ||
|
||
// @ts-expect-error TS is not refining correctly, but we're checking the type | ||
const typeImports: ImportSpecifier[] = node.specifiers.filter( | ||
(i) => i.type === 'ImportSpecifier' && i.importKind === 'type', | ||
); | ||
|
||
// If we have a mix of type and value imports, we need to 'splode them into two import declarations | ||
if (typeImports.length) { | ||
const valueImports = node.specifiers.filter( | ||
(i) => | ||
!(i.type === 'ImportSpecifier' && i.importKind === 'type'), | ||
); | ||
const newValueNode = importDeclaration(valueImports, node.source); | ||
explodedNodes.push(newValueNode); | ||
|
||
// Change the importKind of the specifiers, to avoid `import type {type Foo} from 'foo'` | ||
typeImports.forEach( | ||
(specifier) => (specifier.importKind = 'value'), | ||
); | ||
const newTypeNode = importDeclaration(typeImports, node.source); | ||
newTypeNode.importKind = 'type'; | ||
explodedNodes.push(newTypeNode); | ||
continue; | ||
} | ||
|
||
// Just a boring old values-only node | ||
explodedNodes.push(node); | ||
} | ||
return explodedNodes; | ||
}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
// Jest Snapshot v1, https://goo.gl/fbAQLP | ||
|
||
exports[`imports-with-mixed-declarations.ts - typescript-verify: imports-with-mixed-declarations.ts 1`] = ` | ||
import a, {type LocalType} from './local-file'; | ||
import value, {tp} from 'third-party'; | ||
import {specifier, type ThirdPartyType} from 'third-party'; | ||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
import type { ThirdPartyType } from "third-party"; | ||
|
||
import value, { tp, specifier } from "third-party"; | ||
|
||
import type { LocalType } from "./local-file"; | ||
|
||
import a from "./local-file"; | ||
|
||
`; | ||
|
||
exports[`imports-with-third-party-types.ts - typescript-verify: imports-with-third-party-types.ts 1`] = ` | ||
import a from './local-file'; | ||
import type LocalType from './local-file'; | ||
import value from 'third-party'; | ||
import {specifier} from 'third-party'; | ||
import type ThirdPartyType from 'third-party'; | ||
import type {LocalSpecifierType} from './local-file'; | ||
import type {SpecifierType} from 'third-party'; | ||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
import type ThirdPartyType from "third-party"; | ||
import type { SpecifierType } from "third-party"; | ||
|
||
import value, { specifier } from "third-party"; | ||
|
||
import type LocalType from "./local-file"; | ||
import type { LocalSpecifierType } from "./local-file"; | ||
|
||
import a from "./local-file"; | ||
|
||
`; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
import a, {type LocalType} from './local-file'; | ||
import value, {tp} from 'third-party'; | ||
import {specifier, type ThirdPartyType} from 'third-party'; |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love the "virtual matcher" combined with a custom matcher for Types.
"<TYPES>^[./]"
It feels like a bad precedent to have to split these keys into tags and a regex, and have to match them by
startsWith
, etc.I don't mind the rest of this PR though! We talked about this feature previously.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it does feel a bit wonky. Do you have any idea how else we could support it, though? (We don't actually use
startsWith
here, fwiw, but we do remove the<TYPES>
part when creating the regex.The tricky part here is giving the flexibility to declare a regex as only applying to type imports vs value imports. I think we'd need to support objects in
importOrder
, and use something likeimportOrder: [{regex: '^[./]', isType: true}, ...]
, and that doesn't feel great either. I'm open to suggestions here, though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so thinking about this more deeply, this is supposed to mirror the ability to create arbitrary groups for types. Eg. interleave types and non-type imports in batches.
I'd mayhaps try to argue that as a plugin for an opinionated formatter, maybe we should fight to remove the
importOrder
option entirely, but that seems like a hard sell.Fewer customization points = simpler code, and easier onboarding for users, so I guess your original proposal is better than the alternatives that jump to mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think that it's okay to keep
importOrder
and try to minimize the number of other options that we expose. I definitely think we need a default for it that works in most cases, though.