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: no-deprecated-type-references rule #5509

Merged
merged 7 commits into from
Aug 25, 2022

Conversation

adidahiya
Copy link
Contributor

Changes proposed in this pull request:

New ESLint rule @blueprintjs/no-deprecated-type-references to flag & fix usage of deprecated interfaces in preparation for Blueprint v5.0.

@adidahiya adidahiya requested a review from styu August 23, 2022 19:28
@blueprint-bot
Copy link

finish autofixer

Previews: documentation | landing | table | demo

@blueprint-bot
Copy link

fix lint config

Previews: documentation | landing | table | demo

@@ -26,6 +26,7 @@ module.exports = {
"@blueprintjs/classes-constants": "error",
"@blueprintjs/html-components": "error",
"@blueprintjs/no-deprecated-components": "error",
"@blueprintjs/no-deprecated-type-references": "error",
Copy link
Contributor

Choose a reason for hiding this comment

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

since this is autofixable, can/should we promote it in the default eslint config?

Comment on lines 157 to 165
for (const [packageName, deprecatedTypeNames] of Object.entries(DEPRECATED_TYPE_REFERENCES_BY_PACKAGE)) {
for (const typeName of deprecatedTypeNames) {
const newTypeName = typeName.slice(1);
// "IProps": "Props"
DEPRECATED_TYPES_MAPPING[typeName] = newTypeName;
// "Props": "@blueprintjs/core"
NEW_TYPE_PACKAGE_MAPPING[newTypeName] = packageName;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

i kind of wonder if this is logic we should be running unilaterally as part of importing this rule, or if it should instead be worked into the logic within the rule but without the for loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved it into the create callback

const MySelect = Select.ofType<TimezoneItem>();
`,
},
{
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a test for when there are multiple imports from the same 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.

I tried to do this, but for some reason the rule tester fixer doesn't like to run multiple auto-fixes. so when I write a test like this:

                import { IItemRendererProps, ISelectProps } from "@blueprintjs/select";
                const mySelectProps: ISelectProps = { items: [], renderItem };
                const renderItem = (_item: any, { handleClick, modifiers }: IItemRendererProps) => {
                    return <MenuItem {...modifiers} onClick={handleClick} />;
                };

expecting it to be autofixed to:

                import { ItemRendererProps, SelectProps } from "@blueprintjs/select";
                const mySelectProps: SelectProps = { items: [], renderItem };
                const renderItem = (_item: any, { handleClick, modifiers }: ItemRendererProps) => {
                    return <MenuItem {...modifiers} onClick={handleClick} />;
                };

then it doesn't pass, since the rule tester only runs the first auto-fix. I don't want to write a test with weird expected output showing only one of the deprecated type symbols being fixed. I can look into if this is a bug on ESLint's part...

Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible to test without the fixer, but show that both symbols are flagged as errors instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

discussed offline, ESLint does not allow testing fixable rules without running their fixers. probably another FR for ESLint given the limitations of fixer output in RuleTester (see eslint/eslint#11187 (comment))

@blueprint-bot
Copy link

Merge remote-tracking branch 'origin/develop' into ad/lint-no-deprecated-interfaces

Previews: documentation | landing | table | demo

@blueprint-bot
Copy link

add link to code comment

Previews: documentation | landing | table | demo

@adidahiya adidahiya merged commit 506e18c into develop Aug 25, 2022
@adidahiya adidahiya deleted the ad/lint-no-deprecated-interfaces branch August 25, 2022 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants