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

Upgrade ESLint to 7.5.0 and typescript-eslint to 3.7.1 #2038

Merged
merged 7 commits into from Jul 31, 2020

Conversation

saberduck
Copy link
Contributor

@saberduck saberduck commented Jul 16, 2020

Bump of dependencies

Changes in ruling are related to improvements in eslint

@saberduck
Copy link
Contributor Author

see #2041

@saberduck saberduck force-pushed the tibor/upgrade_eslint7 branch 5 times, most recently from c306b04 to a2f2f21 Compare July 29, 2020 14:51
@saberduck saberduck marked this pull request as ready for review July 30, 2020 07:16
@@ -90,7 +90,7 @@ function reportBadUsage(
break;
case 'ObjectPattern':
node.properties.forEach(prop => {
reportBadUsage(prop.value, buildMessage, context);
reportBadUsage((prop as estree.AssignmentProperty).value, buildMessage, context);
Copy link
Contributor

Choose a reason for hiding this comment

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

so if prop is RestElement we will pass undefined as first argument to reportBadUsage. It will work as expected but I'm not sure such casting to estree.AssignmentProperty is clear about this between-the-lines logic.

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 rewrote this to properly handle RestElement case, it also improves the rule to detect patterns like const {x, ...eval} = foo()

@@ -46,6 +46,8 @@ export const rule: Rule.RuleModule = {
Program: (node: estree.Node) => checkStatements((node as estree.Program).body, context),
BlockStatement: (node: estree.Node) =>
checkStatements((node as estree.BlockStatement).body, context),
TSModuleBlock: (node: estree.Node) =>
checkStatements(((node as unknown) as TSESTree.TSModuleBlock).body as Statement[], context),
Copy link
Contributor

Choose a reason for hiding this comment

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

in fact this is a fix for FN. Do you think it deserves a separate ticket in backlog?

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 think this is minor improvement, let's keep it here

@@ -145,7 +147,7 @@ function raiseInlineAndIndentedIssue(
});
}

function isNestingStatement(node: estree.Node): node is NestingStatement {
function isNestingStatement(node: estree.Node | TSESTree.Node): node is NestingStatement {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this change is required

const sourceCode = context.getSourceCode();
const compositeType = (node as unknown) as
| TSESTree.TSUnionType
| TSESTree.TSIntersectionType;
const groupedTypes: Map<string, Array<TSESTree.Node>> = new Map();

compositeType.types.forEach(typescriptType => {
const nodeValue = sourceCode.getText(typescriptType as estree.Node);
const nodeValue = sourceCode.getText((typescriptType as unknown) as estree.Node);
Copy link
Contributor

Choose a reason for hiding this comment

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

why did it compile before? without as unknown

Copy link
Contributor Author

Choose a reason for hiding this comment

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

something has changed in the type definitions that they don't overlap anymore, I didn't dig deeper

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

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 think some types are out of sync between TSESTree and estree

@@ -91,7 +91,7 @@ function containsShellOption(otherArguments: Argument[]) {
return otherArguments.some(
arg =>
arg.type === 'ObjectExpression' &&
arg.properties.some(
(arg.properties as estree.Property[]).some(
Copy link
Contributor

Choose a reason for hiding this comment

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

what will happen to SpreadElement property?

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 fixed the rule to handle SpreadElement

const TYPESCRIPT_MINIMUM_VERSION = '3.2.1';
const TYPESCRIPT_MAXIMUM_VERSION = '3.8.0';
const TYPESCRIPT_MINIMUM_VERSION = '3.3.1';
const TYPESCRIPT_MAXIMUM_VERSION = '3.10.0';
Copy link
Contributor

Choose a reason for hiding this comment

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

where does this version come from? I see <4.0.0 for typescript-eslint

Copy link
Contributor

Choose a reason for hiding this comment

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

}

@Test
public void test_new_typescript() throws Exception {
File dir = TestUtils.projectDir("tsproject-no-typescript");
TestUtils.npmInstall(dir, "typescript@3.8.0-dev.20191026", "--no-save");
String tsVersion = "4.0.0-beta";
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm, do you think it's an issue? I would keep as is

@@ -69,6 +69,7 @@ public void test() throws Exception {

String projectKey = "tsproject-test-ts-version-" + tsVersion;
SonarScanner build = SonarScanner.create()
.setDebugLogs(true)
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to keep this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, removed

@@ -1,5 +1,5 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

do we have eslint issue for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -0,0 +1,7 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, it will be nice to list in the PR eslint issues causing these changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vilchik-elena vilchik-elena added this to the 6.4 milestone Jul 30, 2020

// this value is taken from typescript-estree
// still we might consider extending this range
// if everything which we need is working on older/newer versions
const TYPESCRIPT_MINIMUM_VERSION = '3.2.1';
const TYPESCRIPT_MAXIMUM_VERSION = '3.8.0';
const TYPESCRIPT_MINIMUM_VERSION = '3.3.1';
Copy link
Contributor

Choose a reason for hiding this comment

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

we should update documentation.md

@vilchik-elena vilchik-elena changed the title Upgrade to Eslint7 Upgrade ESLint to 7 Jul 30, 2020
@vilchik-elena vilchik-elena changed the title Upgrade ESLint to 7 Upgrade ESLint to 7.5.0 and typescript-eslint to 3.7.1 Jul 30, 2020
@saberduck saberduck force-pushed the tibor/upgrade_eslint7 branch 2 times, most recently from 9853780 to aa09234 Compare July 30, 2020 20:56
@saberduck saberduck mentioned this pull request Jul 30, 2020
Copy link
Contributor

@vilchik-elena vilchik-elena left a comment

Choose a reason for hiding this comment

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

LGTM, but remove unused config for test without tsconfig.
Also we should find eslint tickets in rules S2814 and S121

@@ -62,13 +62,6 @@ class RuleTesterTs extends RuleTester {
invalid?: RuleTester.InvalidTestCase[];
},
): void {
if (this.expectNoIssuesWithoutTypes) {
this.ruleTesterNoTsConfig.run(`${name}[noTsConfig]`, rule, {
Copy link
Contributor

Choose a reason for hiding this comment

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

@saberduck not handled

@sonarsource-next
Copy link

Kudos, SonarQube Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@vilchik-elena vilchik-elena merged commit dc7eb58 into master Jul 31, 2020
@vilchik-elena vilchik-elena deleted the tibor/upgrade_eslint7 branch July 31, 2020 09:01
@vilchik-elena vilchik-elena mentioned this pull request Aug 10, 2020
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

2 participants