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.

Sign up for GitHub

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

Jump to bottom

fix(eslint-plugin): [keyword-spacing] unexpected space before/after in import type #6095

Merged
merged 7 commits into from Dec 2, 2022
Merged

fix(eslint-plugin): [keyword-spacing] unexpected space before/after in import type #6095

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
78f3749
fix/issue6063-keyword-spacing-unexpected-space-before
Nov 26, 2022
91c18ff
remove some redundancy
Nov 26, 2022
4d58a46
Merge remote-tracking branch 'upstream/main' into fix/issue6063-keywo…
Nov 26, 2022
ac5f9bd
Add overrides for type token
Nov 27, 2022
78a634c
Merge remote-tracking branch 'upstream/main' into fix/issue6063-keywo…
Nov 27, 2022
e70260a
Merge remote-tracking branch 'upstream/main' into fix/issue6063-keywo…
Dec 1, 2022
e8d5efc
test
Dec 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
55 changes: 43 additions & 12 deletions packages/eslint-plugin/src/rules/keyword-spacing.ts
View file
Open in desktop
@@ -1,5 +1,5 @@
import type { TSESTree } from '@typescript-eslint/utils';
import { AST_TOKEN_TYPES } from '@typescript-eslint/utils';
import { AST_NODE_TYPES, AST_TOKEN_TYPES } from '@typescript-eslint/utils';

import * as util from '../util';
import { getESLintCoreRule } from '../util/getESLintCoreRule';
Expand All @@ -9,6 +9,25 @@ const baseRule = getESLintCoreRule('keyword-spacing');
export type Options = util.InferOptionsTypeFromRule<typeof baseRule>;
export type MessageIds = util.InferMessageIdsTypeFromRule<typeof baseRule>;

// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
const baseSchema = Array.isArray(baseRule.meta.schema)
? baseRule.meta.schema[0]
: baseRule.meta.schema;
const schema = util.deepMerge(
// eslint-disable-next-line @typescript-eslint/no-unsafe-argument -- https://github.com/microsoft/TypeScript/issues/17002
baseSchema,
{
properties: {
overrides: {
properties: {
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-member-access
type: baseSchema.properties.overrides.properties.import,
},
},
},
},
);

export default util.createRule<Options, MessageIds>({
name: 'keyword-spacing',
meta: {
Expand All @@ -20,12 +39,12 @@ export default util.createRule<Options, MessageIds>({
},
fixable: 'whitespace',
hasSuggestions: baseRule.meta.hasSuggestions,
schema: baseRule.meta.schema,
schema: [schema],
messages: baseRule.meta.messages,
},
defaultOptions: [{}],

create(context, [{ after }]) {
create(context, [{ after, overrides }]) {
const sourceCode = context.getSourceCode();
const baseRules = baseRule.create(context);
return {
Expand Down Expand Up @@ -54,25 +73,37 @@ export default util.createRule<Options, MessageIds>({
'ImportDeclaration[importKind=type]'(
node: TSESTree.ImportDeclaration,
): void {
const { type: typeOptionOverride = {} } = overrides ?? {};
const typeToken = sourceCode.getFirstToken(node, { skip: 1 })!;
const punctuatorToken = sourceCode.getTokenAfter(typeToken)!;
if (
node.specifiers?.[0]?.type === AST_NODE_TYPES.ImportDefaultSpecifier
) {
return;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought draft means you won't review it (Sorry, I guess it meant something else to me).

I opened it for review because I can't think of a way to solve it without adding an override option for the type keyword or should it use the existing option for the import keyword.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I just didn't realize there was a question associated with this! Sorry, i misinterpreted the discussion. Will think on it 🙂

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, actually, definitely yes. Since the type option doesn't exist in ESLint core and is TypeScript-specific, we should add in a feature for it. Yes please 😄

Copy link
Contributor Author

@omril1 omril1 Nov 26, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any example I can look at how to do that? The types seem looser than the schema validation (typed as overrides?: Record<string, { before?: boolean; after?: boolean;}> but adding type: { after: true } to object breaks the schema validation

OK, I found something similar to the no-magic-numbers rule

const spacesBetweenTypeAndPunctuator =
punctuatorToken.range[0] - typeToken.range[1];
if (after && spacesBetweenTypeAndPunctuator === 0) {
if (
(typeOptionOverride.after ?? after) === true &&
spacesBetweenTypeAndPunctuator === 0
) {
context.report({
loc: punctuatorToken.loc,
messageId: 'expectedBefore',
data: { value: punctuatorToken.value },
loc: typeToken.loc,
messageId: 'expectedAfter',
data: { value: 'type' },
fix(fixer) {
return fixer.insertTextBefore(punctuatorToken, ' ');
return fixer.insertTextAfter(typeToken, ' ');
},
});
}
if (!after && spacesBetweenTypeAndPunctuator > 0) {
if (
(typeOptionOverride.after ?? after) === false &&
spacesBetweenTypeAndPunctuator > 0
) {
context.report({
loc: punctuatorToken.loc,
messageId: 'unexpectedBefore',
data: { value: punctuatorToken.value },
loc: typeToken.loc,
messageId: 'unexpectedAfter',
data: { value: 'type' },
fix(fixer) {
return fixer.removeRange([
typeToken.range[1],
Expand Down
152 changes: 146 additions & 6 deletions packages/eslint-plugin/tests/rules/keyword-spacing.test.ts
View file
Open in desktop
Expand Up @@ -116,12 +116,122 @@ ruleTester.run('keyword-spacing', rule, {
},
{
code: 'import type { foo } from "foo";',
options: [{ overrides: { as: {} } }],
options: [BOTH],
parserOptions: { ecmaVersion: 6, sourceType: 'module' },
},
{
code: "import type * as Foo from 'foo'",
options: [{ overrides: { as: {} } }],
options: [BOTH],
parserOptions: { ecmaVersion: 6, sourceType: 'module' },
},
{
code: 'import type { SavedQueries } from "./SavedQueries.js";',
options: [
{
before: true,
after: false,
overrides: {
else: { after: true },
return: { after: true },
try: { after: true },
catch: { after: false },
case: { after: true },
const: { after: true },
throw: { after: true },
let: { after: true },
do: { after: true },
of: { after: true },
as: { after: true },
finally: { after: true },
from: { after: true },
import: { after: true },
export: { after: true },
default: { after: true },
// The new option:
type: { after: true },
},
},
],
parserOptions: { ecmaVersion: 6, sourceType: 'module' },
},
{
// Space after import is not configurable from option since it's invalid syntax with import type
code: 'import type { SavedQueries } from "./SavedQueries.js";',
options: [
{
before: true,
after: true,
overrides: {
import: { after: false },
},
},
],
parserOptions: { ecmaVersion: 6, sourceType: 'module' },
},
{
code: "import type{SavedQueries} from './SavedQueries.js';",
options: [
{
before: true,
after: false,
overrides: {
from: { after: true },
},
},
],
parserOptions: { ecmaVersion: 6, sourceType: 'module' },
},
{
code: "import type{SavedQueries} from'./SavedQueries.js';",
options: [
{
before: true,
after: false,
},
],
parserOptions: { ecmaVersion: 6, sourceType: 'module' },
},
{
code: "import type http from 'node:http';",
options: [
{
before: true,
after: false,
overrides: {
from: { after: true },
},
},
],
parserOptions: { ecmaVersion: 6, sourceType: 'module' },
},
{
code: "import type http from'node:http';",
options: [
{
before: true,
after: false,
},
],
parserOptions: { ecmaVersion: 6, sourceType: 'module' },
},
{
code: 'import type {} from "foo";',
options: [BOTH],
parserOptions: { ecmaVersion: 6, sourceType: 'module' },
},
{
code: 'import type { foo1, foo2 } from "foo";',
options: [BOTH],
parserOptions: { ecmaVersion: 6, sourceType: 'module' },
},
{
code: 'import type { foo1 as _foo1, foo2 as _foo2 } from "foo";',
options: [BOTH],
parserOptions: { ecmaVersion: 6, sourceType: 'module' },
},
{
code: 'import type { foo as bar } from "foo";',
options: [BOTH],
parserOptions: { ecmaVersion: 6, sourceType: 'module' },
},
],
Expand Down Expand Up @@ -167,28 +277,58 @@ ruleTester.run('keyword-spacing', rule, {
output: 'import type { foo } from "foo";',
options: [{ after: true, before: true }],
parserOptions: { ecmaVersion: 6, sourceType: 'module' },
errors: [{ messageId: 'expectedBefore', data: { value: '{' } }],
errors: expectedAfter('type'),
},
{
code: 'import type { foo } from"foo";',
output: 'import type{ foo } from"foo";',
options: [{ after: false, before: true }],
parserOptions: { ecmaVersion: 6, sourceType: 'module' },
errors: [{ messageId: 'unexpectedBefore', data: { value: '{' } }],
errors: unexpectedAfter('type'),
},
{
code: 'import type* as foo from "foo";',
output: 'import type * as foo from "foo";',
options: [{ after: true, before: true }],
parserOptions: { ecmaVersion: 6, sourceType: 'module' },
errors: [{ messageId: 'expectedBefore', data: { value: '*' } }],
errors: expectedAfter('type'),
},
{
code: 'import type * as foo from"foo";',
output: 'import type* as foo from"foo";',
options: [{ after: false, before: true }],
parserOptions: { ecmaVersion: 6, sourceType: 'module' },
errors: [{ messageId: 'unexpectedBefore', data: { value: '*' } }],
errors: unexpectedAfter('type'),
},
{
code: "import type {SavedQueries} from './SavedQueries.js';",
output: "import type{SavedQueries} from './SavedQueries.js';",
options: [
{
before: true,
after: false,
overrides: {
from: { after: true },
},
},
],
parserOptions: { ecmaVersion: 6, sourceType: 'module' },
errors: unexpectedAfter('type'),
},
{
code: "import type {SavedQueries} from './SavedQueries.js';",
output: "import type{SavedQueries} from'./SavedQueries.js';",
options: [
{
before: true,
after: false,
},
],
parserOptions: { ecmaVersion: 6, sourceType: 'module' },
errors: [
{ messageId: 'unexpectedAfter', data: { value: 'type' } },
{ messageId: 'unexpectedAfter', data: { value: 'from' } },
],
},
],
});