Add basic eslint recommendations to `core/` #3077

edmundito · 2022-03-03T15:26:57Z

Change description

Based on the issue in #3070 that could have been prevented with linting, this adds the most basic eslint rules to core and fixes a number of existing issues. The rules are not as strict as our plan because there's currently a lot to fix, and the plan is to incrementally enable rules and fix issues as we find the time to do it.

Checklists

Development

Application changes have been tested appropriately

Impact

Code follows company security practices and guidelines
Security impact of change has been considered
Performance impact of change has been considered
Possible migration needs considered (model migrations, config migrations, etc.)

Please explain any security, performance, migration, or other impacts if relevant:

Code review

Pull request has a descriptive title and context useful to a reviewer. Screenshots or screencasts are attached where applicable.
Relevant tags have been added to the PR (bug, enhancement, internal, etc.)

edmundito · 2022-03-03T15:28:41Z

core/.eslintrc.js

+  },
+  extends: [
+    "eslint:recommended",
+    "plugin:@typescript-eslint/eslint-recommended",


Notice here that I'm using @typescript-eslint/eslint-recommended instead of @typescript-eslint/recommended. This extension only makes sure that typescript plays nicely with eslint:recommended but does not enable the typescript recommended rules. Currently, there are a gargantuan number of problems with core so doing it all once is a herculean effort. At the very least we're getting base safety.

edmundito · 2022-03-03T15:31:44Z

core/.eslintrc.js

+  globals: {
+    fetch: true, // For isomorphic-fetch
+  },
+  ignorePatterns: ["__tests__", "dist", "public"],


Lots of problems in __tests__ to fix, so it's being ignored for this PR.

edmundito · 2022-03-03T15:33:16Z

core/src/modules/broadcastHelper.ts

+  } catch {
+    // Ignore error
+  }


In the recommendations we get the no-empty rule that discourages empty blocks. The easiest way to fix it is to put a comment.

edmundito · 2022-03-03T15:36:23Z

core/src/modules/cloudCli.ts

@@ -74,7 +74,7 @@ export namespace CloudCLI {
      );

      let lastState: string;
-      while (true) {
+      for (;;) {


Fix for no-constant-condition

Do you think we should disable that rule? We could rewrite this function to not need an infinite loop but I'm not it's worth the refactor time.

evantahler

Nice!

these look to me mostly stylistic changes… did you see any logical problems?

edmundito · 2022-03-03T15:37:36Z

core/src/modules/configLoaders/schedule.ts

@@ -20,7 +20,7 @@ export async function loadSchedule(
 ): Promise<IdsByClass> {
  let isNew = false;

-  if (configObject.hasOwnProperty("confirmProfiles")) {
+  if (Object.prototype.hasOwnProperty.call(configObject, "confirmProfiles")) {


no-prototype-builtins

edmundito · 2022-03-03T15:38:29Z

core/src/modules/ops/record.ts

@@ -260,6 +258,7 @@ export namespace RecordOps {
    recordProperties: {
      [key: string]: (string | number | boolean | Date)[] | any;
    }[],
+    // eslint-disable-next-line @typescript-eslint/no-unused-vars
    toLock = true,


This argument is not really being used right now, so I'm not sure whether to remove it and update all the instances that use it or leave it.

If there's enough of these I'd say icebox a ticket to remove.

edmundito · 2022-03-03T15:39:09Z

core/src/modules/ops/source.ts

-    } catch (error) {
-      throw error;


Unnecessary catches because they only rethrow the error

edmundito · 2022-03-03T15:40:17Z

core/src/modules/reset.ts

@@ -28,6 +28,7 @@ export namespace Reset {
   * * clears the redis cache
   * * clears resque
   */
+  // eslint-disable-next-line @typescript-eslint/no-unused-vars


I left these here because the test is definitely passing a callerId. But callerId is not used in any of these functions. I could update the usage of these so that callerId is no longer used and leave it for some future use.

🤷 Probably not worth removing

edmundito · 2022-03-03T15:45:11Z

core/src/tasks/system/status/status.ts

@@ -81,6 +81,7 @@ export class StatusTask extends CLSTask {
    if (!toStop) return false;

    // do not await so the promise can end so the server can shut down
+    // eslint-disable-next-line no-async-promise-executor


It really does not like returning a promise with an async call. Looks like this is intentional but could be dangerous otherwise. Doc explains why:

no-async-promise-executor

The proimise isn't returned. Does:

await api.process.stop(); process.nextTick(() => process.exit(0));

Even need to be wrapped?

I feel like maybe the entire block could be api.process.stop().then(() => process.nextTick(() => process.exit(0)));
@evantahler Thoughts?

edmundito · 2022-03-03T15:48:53Z

@evantahler The style issues are always easy to fix. I highlighted a few areas in the comments that point out potential logic issues.

krishnaglick

I expect some merge conflicts with my just-merged code.

krishnaglick · 2022-03-03T16:07:04Z

core/.eslintrc.js

+  },
+  overrides: [
+    {
+      files: ["*.ts", "*.tsx"],


I think you could just add *.js to ignorePatterns and you shouldn't need this?

This gets added by @typescript:recommended but not @typescript:eslint-recommended So I need to manually add this override otherwise I have to run eslint with eslint . --ext ts,tsx

krishnaglick · 2022-03-03T16:10:41Z

core/src/modules/cloudCli.ts

@@ -74,7 +74,7 @@ export namespace CloudCLI {
      );

      let lastState: string;
-      while (true) {
+      for (;;) {


Do you think we should disable that rule? We could rewrite this function to not need an infinite loop but I'm not it's worth the refactor time.

krishnaglick · 2022-03-03T16:12:01Z

core/src/modules/oAuth.ts

-    } catch (e) {
-      throw e;
+      this.accessToken = token;
+      this.expirationDate = Date.now() + expirationSeconds * 1000;


Unwrapped 👍

krishnaglick · 2022-03-03T16:14:41Z

core/src/tasks/system/status/status.ts

@@ -81,6 +81,7 @@ export class StatusTask extends CLSTask {
    if (!toStop) return false;

    // do not await so the promise can end so the server can shut down
+    // eslint-disable-next-line no-async-promise-executor


The proimise isn't returned. Does:

await api.process.stop(); process.nextTick(() => process.exit(0));

Even need to be wrapped?

I feel like maybe the entire block could be api.process.stop().then(() => process.nextTick(() => process.exit(0)));
@evantahler Thoughts?

krishnaglick · 2022-03-03T16:15:12Z

core/src/modules/statusReporters.ts

@@ -528,7 +527,7 @@ function mergeMetrics(metrics: StatusMetric[]) {
      mergedMetrics.push(item);
    } else if (idx !== null) {
      for (const k in Object.keys(item)) {
-        if (item.hasOwnProperty(k)) {
+        if (Object.prototype.hasOwnProperty.call(item, k)) {


Don't love this rule

Yeah, good discussion point. Should we allow it? Should we not use hasOwnProperty?

Looks like hasOwn exists and we should maybe just use that instead?

hasOwn is super new. It looks like we'll have to upgrade to target es2022, we're still targeting es2019.
https://devblogs.microsoft.com/typescript/announcing-typescript-4-6/#target-es2022

I don't object :P

krishnaglick · 2022-03-03T16:16:39Z

core/src/modules/reset.ts

@@ -28,6 +28,7 @@ export namespace Reset {
   * * clears the redis cache
   * * clears resque
   */
+  // eslint-disable-next-line @typescript-eslint/no-unused-vars


🤷 Probably not worth removing

krishnaglick · 2022-03-03T16:21:21Z

core/src/modules/ops/record.ts

@@ -260,6 +258,7 @@ export namespace RecordOps {
    recordProperties: {
      [key: string]: (string | number | boolean | Date)[] | any;
    }[],
+    // eslint-disable-next-line @typescript-eslint/no-unused-vars
    toLock = true,


If there's enough of these I'd say icebox a ticket to remove.

…rrent issues

edmundito added the internal chores and housekeeping label Mar 3, 2022

edmundito commented Mar 3, 2022

View reviewed changes

evantahler reviewed Mar 3, 2022

View reviewed changes

edmundito commented Mar 3, 2022

View reviewed changes

edmundito requested a review from krishnaglick March 3, 2022 15:40

edmundito commented Mar 3, 2022

View reviewed changes

krishnaglick reviewed Mar 3, 2022

View reviewed changes

edmundito force-pushed the eslint-core branch 2 times, most recently from ff4630e to a90add8 Compare March 7, 2022 14:22

edmundito requested a review from evantahler March 7, 2022 14:25

edmundito added 6 commits March 10, 2022 09:04

Add eslint rules to core with basic recommendation enables and fix cu…

9ac34c6

…rrent issues

Update eslintrc to ignore public and add array-type rule

1b19599

Update eslintrc with typescript consistent-indexed-object-style warnings

89e0a6c

Reverse move function for now disabled no-inner-declarations eslint rule

fdcf84f

Remove todo from eslint rules

74caa34

Fix consistent-indexed-object-style

a266074

edmundito force-pushed the eslint-core branch from a90add8 to a266074 Compare March 10, 2022 14:09

evantahler removed their request for review March 15, 2022 16:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic eslint recommendations to `core/` #3077

Add basic eslint recommendations to `core/` #3077

edmundito commented Mar 3, 2022

edmundito Mar 3, 2022

edmundito Mar 3, 2022

edmundito Mar 3, 2022 •

edited

edmundito Mar 3, 2022

krishnaglick Mar 3, 2022

evantahler left a comment

edmundito Mar 3, 2022

edmundito Mar 3, 2022

krishnaglick Mar 3, 2022

edmundito Mar 3, 2022

edmundito Mar 3, 2022

krishnaglick Mar 3, 2022

edmundito Mar 3, 2022

krishnaglick Mar 3, 2022

edmundito commented Mar 3, 2022

krishnaglick left a comment

krishnaglick Mar 3, 2022

edmundito Mar 3, 2022

krishnaglick Mar 3, 2022

krishnaglick Mar 3, 2022

krishnaglick Mar 3, 2022

krishnaglick Mar 3, 2022

edmundito Mar 3, 2022

krishnaglick Mar 3, 2022

edmundito Mar 3, 2022 •

edited

krishnaglick Mar 3, 2022

krishnaglick Mar 3, 2022

krishnaglick Mar 3, 2022

Add basic eslint recommendations to core/ #3077

Are you sure you want to change the base?

Add basic eslint recommendations to core/ #3077

Conversation

edmundito commented Mar 3, 2022

Change description

Checklists

Development

Impact

Code review

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edmundito Mar 3, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evantahler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edmundito commented Mar 3, 2022

krishnaglick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edmundito Mar 3, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add basic eslint recommendations to `core/` #3077

Add basic eslint recommendations to `core/` #3077

edmundito Mar 3, 2022 •

edited

edmundito Mar 3, 2022 •

edited