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

[eslint-plugin] feat: only lint MenuItem with popoverProps attr #5495

Merged
merged 7 commits into from
Aug 11, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,15 @@ import { TSESLint, TSESTree } from "@typescript-eslint/utils";

import { createRule } from "../utils/createRule";

type MessageIds = "migration";
type MessageIds = "migration" | "migrationWithPropUsage";

/**
* Higher-order function to create an ESLint rule which checks for usage of deprecated React components
* in JSX syntax.
*
* Only components imported from `packagesToCheck` will be flagged. The lint violation will include
* a recommendation to migrate to the newer, non-deprecated component (this must be specified for each
* component via the second argument `deprecatedToNewComponentMapping`).
* @param packagesToCheck Only components imported from these packages will be flagged.
* @param deprecatedToNewComponentMapping Lint violations will include a recommendation to migrate to the newer,
* non-deprecated component specified in this mapping. Keys in this object may use
*/
export function createNoDeprecatedComponentsRule(
ruleName: string,
Expand All @@ -37,11 +37,14 @@ export function createNoDeprecatedComponentsRule(
recommended: "error",
},
messages: {
migration: "{{ deprecatedComponentName }} is deprecated, migrate to {{ newComponentName }} instead",
migration:
"Usage of {{ deprecatedComponentName }} is deprecated, migrate to {{ newComponentName }} instead",
migrationWithPropUsage:
"Usage of {{ deprecatedComponentName }} with prop '{{ deprecatedPropName }}' is deprecated, migrate to {{ newComponentName }} instead",
},
schema: [
{
enum: ["migration"],
enum: ["migration", "migrationWithPropUsage"],
},
],
},
Expand All @@ -52,24 +55,35 @@ export function createNoDeprecatedComponentsRule(
| { type: "function"; functionName: string; localFunctionName: string }
> = [];

// parses out additional deprecated components from entries like { "MenuItem.popoverProps": "MenuItem2" }
const additionalDeprecatedComponents = Object.keys(deprecatedToNewComponentMapping).reduce<string[]>(
(components, key) => {
const [componentName, propName] = key.split(".");
if (propName !== undefined) {
components.push(componentName);
}
return components;
},
[],
);

function isDeprecatedComponent(name: string) {
if (deprecatedImports.length === 0) {
return false;
}

return deprecatedImports.some(
deprecatedImport =>
(deprecatedImport.type === "function" && deprecatedImport.localFunctionName === name) ||
deprecatedToNewComponentMapping[name] != null,
return (
deprecatedToNewComponentMapping[name] != null &&
deprecatedImports.some(
deprecatedImport =>
deprecatedImport.type === "function" && deprecatedImport.localFunctionName === name,
)
);
}

function isDeprecatedNamespacedComponent(name: string, property: string) {
return (
deprecatedToNewComponentMapping[property] != null &&
deprecatedImports.some(
deprecatedImport =>
deprecatedImport.type === "namespace" && deprecatedImport.namespace === name,
) && deprecatedToNewComponentMapping[property] != null
)
);
}

Expand All @@ -89,7 +103,10 @@ export function createNoDeprecatedComponentsRule(
});
break;
case TSESTree.AST_NODE_TYPES.ImportSpecifier:
if (deprecatedToNewComponentMapping[importClause.imported.name] != null) {
if (
deprecatedToNewComponentMapping[importClause.imported.name] != null ||
additionalDeprecatedComponents.includes(importClause.imported.name)
) {
deprecatedImports.push({
functionName: importClause.imported.name,
localFunctionName: importClause.local.name,
Expand All @@ -101,7 +118,7 @@ export function createNoDeprecatedComponentsRule(
}
},

// check <DeprecatedComponent /> syntax
// check <DeprecatedComponent> syntax (includes self-closing tags)
"JSXElement > JSXOpeningElement > JSXIdentifier": (node: TSESTree.JSXIdentifier) => {
if (isDeprecatedComponent(node.name)) {
context.report({
Expand All @@ -112,10 +129,34 @@ export function createNoDeprecatedComponentsRule(
messageId: "migration",
node,
});
} else if (isOpeningElement(node.parent)) {
// check <DeprecatedComponent withDeprecatedProp={...}> syntax
const deprecatedProp = node.parent.attributes.find(
attribute =>
attribute.type === TSESTree.AST_NODE_TYPES.JSXAttribute &&
attribute.name.type === TSESTree.AST_NODE_TYPES.JSXIdentifier &&
deprecatedToNewComponentMapping[`${node.name}.${attribute.name.name}`] != null,
);
if (deprecatedProp !== undefined) {
adidahiya marked this conversation as resolved.
Show resolved Hide resolved
const deprecatedComponentKey = Object.keys(deprecatedToNewComponentMapping).find(
key => key.split(".")[0] === node.name,
Copy link
Contributor

Choose a reason for hiding this comment

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

it is technically possible to have both MenuItem and MenuItem.popoverProps as keys, in which case this would return the wrong key, right?

Suggested change
key => key.split(".")[0] === node.name,
key => key.indexOf(".") >= 0 && key.split(".")[0] === node.name,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think it would fail in that case. The rule doesn't gracefully handle bad configuration right now, but I figure it's ok to be a little simplistic here since the configuration is not exposed to consumers -- it only lives inside the rule implementation in this package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed it up a little to guard against the edge case you mentioned

);
context.report({
data: {
deprecatedComponentName: node.name,
deprecatedPropName: (
(deprecatedProp as TSESTree.JSXAttribute).name as TSESTree.JSXIdentifier
).name,
newComponentName: deprecatedToNewComponentMapping[deprecatedComponentKey!],
},
messageId: "migrationWithPropUsage",
node,
});
}
}
},

// check <Blueprint.DeprecatedComponent /> syntax
// check <Blueprint.DeprecatedComponent> syntax (includes self-closing tags)
"JSXElement > JSXOpeningElement > JSXMemberExpression[property.type='JSXIdentifier']": (
node: TSESTree.JSXMemberExpression,
) => {
Expand All @@ -137,6 +178,30 @@ export function createNoDeprecatedComponentsRule(
messageId: "migration",
node: node.property,
});
} else if (isOpeningElement(node.parent)) {
// check <Blueprint.DeprecatedComponent withDeprecatedProp={...}> syntax
const deprecatedProp = node.parent.attributes.find(
attribute =>
attribute.type === TSESTree.AST_NODE_TYPES.JSXAttribute &&
attribute.name.type === TSESTree.AST_NODE_TYPES.JSXIdentifier &&
deprecatedToNewComponentMapping[`${node.property.name}.${attribute.name.name}`] != null,
);
if (deprecatedProp !== undefined) {
const deprecatedComponentKey = Object.keys(deprecatedToNewComponentMapping).find(
key => key.split(".")[0] === node.property.name,
);
context.report({
data: {
deprecatedComponentName: node.property.name,
deprecatedPropName: (
(deprecatedProp as TSESTree.JSXAttribute).name as TSESTree.JSXIdentifier
).name,
newComponentName: deprecatedToNewComponentMapping[deprecatedComponentKey!],
},
messageId: "migrationWithPropUsage",
node,
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

is this code repeated, just for a different node type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's a little different, it uses node.property.name instead of node.name. but yes it's very similar to the other block. I think it would be a little convoluted / unreadable if we tried to DRY this code between the two syntax handlers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

on second thought, I've refactored this bit into a function shared between the two handlers, it's still legible

}
},

Expand Down Expand Up @@ -182,3 +247,7 @@ export function createNoDeprecatedComponentsRule(
},
});
}

function isOpeningElement(parent: TSESTree.Node | undefined): parent is TSESTree.JSXOpeningElement {
return parent?.type === TSESTree.AST_NODE_TYPES.JSXOpeningElement;
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ export const coreComponentsMigrationMapping = {
AbstractPureComponent: "AbstractPureComponent2",
Breadcrumbs: "Breadcrumbs2",
CollapsibleList: "OverflowList",
MenuItem: "MenuItem2",
"MenuItem.popoverProps": "MenuItem2",
PanelStack: "PanelStack2",
Popover: "Popover2",
Tooltip: "Tooltip2",
Expand Down
1 change: 1 addition & 0 deletions packages/eslint-plugin/test/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,4 @@ import "./classes-constants.test";
import "./html-components.test";
import "./icon-components.test";
import "./no-deprecated-components.test";
import "./no-deprecated-core-components.test";
91 changes: 91 additions & 0 deletions packages/eslint-plugin/test/no-deprecated-core-components.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
/*
* Copyright 2022 Palantir Technologies, Inc. All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

// tslint:disable object-literal-sort-keys
/* eslint-disable no-template-curly-in-string */

import { TSESLint } from "@typescript-eslint/utils";
import dedent from "dedent";

import { noDeprecatedCoreComponentsRule } from "../src/rules/no-deprecated-components";

const ruleTester = new TSESLint.RuleTester({
parser: require.resolve("@typescript-eslint/parser"),
parserOptions: {
ecmaFeatures: {
jsx: true,
},
sourceType: "module",
},
});

console.info("Testing no-deprecated-core-components rule...");
ruleTester.run("no-deprecated-core-components", noDeprecatedCoreComponentsRule, {
// N.B. most other deprecated components are tested by no-deprecated-components.test.ts, this suite just tests
// for more specific violations which involve certain deprecated props
invalid: [
{
code: dedent`
import { MenuItem } from "@blueprintjs/core";

return <MenuItem popoverProps={{ boundary: "window" }} />
`,
errors: [
{
messageId: "migrationWithPropUsage",
data: {
deprecatedComponentName: "MenuItem",
deprecatedPropName: "popoverProps",
newComponentName: "MenuItem2",
},
},
],
},
{
code: dedent`
import * as Blueprint from "@blueprintjs/core";

return <Blueprint.MenuItem popoverProps={{ boundary: "window" }} />
`,
errors: [
{
messageId: "migrationWithPropUsage",
data: {
deprecatedComponentName: "MenuItem",
deprecatedPropName: "popoverProps",
newComponentName: "MenuItem2",
},
},
],
},
],
valid: [
{
code: dedent`
import { MenuItem } from "@blueprintjs/core";

return <MenuItem text="Open in new tab" icon="share" />
`,
},
{
code: dedent`
import * as Blueprint from "@blueprintjs/core";

return <Blueprint.MenuItem text="Open in new tab" icon="share" />
`,
},
],
});