Proposed linting config updates #669

aoberoi · 2020-10-18T21:14:02Z

Summary

This PR explores using a linting configuration that is different in a few key ways:

Less strict formatting rules, by removing Prettier. Another way of saying this is authors have more control of formatting. Rules related to best practices and finding logical and syntactical issues are just as strict, if not, more strict.
- This should improve the behavior of editors (such as VSCode) by dramatically simplifying use of extensions/plugins. Prettier extensions will leave this codebase alone, and ESLint plugins will correct issues that have automatic fixers on save.
Fully commented to communicate the meaning and intention of rules. We want source authors to understand the benefit of the rules instead of feeling like they need to obey an arbitrary robot.
- We'd like the rules to be accessible for discussing new opinions and ideas. That can only happen if the reasoning for existing rules is explained.
Generalized to work well on JavaScript, TypeScript, and mixed source codebases. Once this config has had time to bake in a single working codebase, like this one, the config can be extracted into its own package and applied to all JS/TS projects in slackapi.
Borrows as much as possible from well-considered community style guides such as AirBnB's JavaScript style guide, ESLint's recommended rules, Node.js recommended rules, JSDoc recommended rules
- Applies new rules, or overrides existing rules, only when we have an opinion that differs for specific reasons.
Config is contained in one file, instead of spread out in several directories and files.
- Structured using overrides to create an obvious space for additional tweaks based on their desired affects. For example, it's clear where to add new rules that only apply to TypeScript, new rules that apply to both TypeScript and JavaScript, or a rule configuration that overrides an existing rule from the above configs.

Disadvantages

The main disadvantage of this config is the loss of the indent rule within TypeScript. The author of the TypeScript-specific fork of that rule has decided not to maintain it any longer, and has acknowledged that the rule currently in a broken state. The author prefers to use Prettier in their own work, which relieved them of the need to work on that rule. But Prettier is highly opinionated as a formatter, and not nearly as configurable, so in this change we've made the tradeoff that this flexibility is worth the cost of not having a good indent rule in *.ts files.

The next largest disadvantage is lack of enforcement on JSDoc descriptions for all public API. This isn't a regression though, the current config also doesn't enforce this.

A few other rules are less than perfect, but those shortcomings have been described in the comments. Where possible, a link to relevant issues to watch have been added.

Feedback needed

The src/App.ts file was migrated to pass this config in order to aide discussion of this config. Please look at the changes in this one file, and let us know if any changes don't sit well with you. Feel free to be as opinionated or subjective as you want. Now is a good time to get aligned. After that, it should be relatively straight-forward to update the rest of the source files to pass.

One particular rule I'm not too sure about is the preference for default exports. It seems to go against the direction which React is now moving towards intentionally. I believe the reasoning is that named exports are more compatible across CommonJS and ESM module formats without any runtime/transpiler support. Should this matter to us?

Are any of the disadvantages dealbreakers?

TODO

Update config for *.spec.js and *.spec.ts files, to describe unique environment (mocha globals) and conventions.
Do we need any unique/specific rules for type-tests?
Migrate the existing sources files to satisfy these rules (besides src/App.ts). This should happen after other maintainers have offered feedback.
Update the npm scripts in package.json, re-enable format on save for VSCode in the settings.

Requirements (place an `x` in each `[ ]`)

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.

what if we remove dependency on Prettier, and solely rely on ESLint? it seems we will lose some strictness in our enforcement, but possibly gain a better development experience in this codebase. Prettier reprints the entire program following its own style (subject to a few config options) but its style is often subpar for our needs. it also can be tricky for IDEs, extensions, and CI to integrate with. we've had our fair share of issues and exploring this direction will allow us to understand what we might gain and whether its worth it.

TODO: lint rules for test files (*.spec.ts) starting off with some well considered and inherited rules. will evaluate the impact of these rules on a file or two to see how well they hold up.

there are still warnings in the file, and that's okay for now. we're getting pretty close a good spot to collect feedback.

stevengill

Did a first pass of the eslintrc and App.ts. Still need to run it locally.

.eslintrc.js

stevengill · 2020-10-19T22:48:22Z

.eslintrc.js

+        // no reason we cannot adopt this syntax now.
+        // NOTE: The `@typescript-eslint/no-require-imports` rule can also achieve the same effect, but it is less
+        // configurable and only built to provide a migration path from TSLint.
+        'import/no-commonjs': ['error', {


can this rule still be overridden inline? I've seen a few places in our code that require('package.json'). Importing it instead breaks our dist

good question! let me know if it breaks the dist after you try. not sure if this is what you've tried in the past, but i also updated the tsconfig.json to turn on the resolveJsonModule option. that might make the difference.

I used resolveJsonModule option last time I tried. Just pulled this locally and this issue exists. You can see it by running npm pack in your root directly (the command npm publish uses under the hood to build the archive). The package that is created is missing all of dist pretty much.

.eslintrc.js

stevengill · 2020-10-19T22:51:08Z

.eslintrc.js

+
+        // Allow cyclical imports. Turning this rule on is mainly a way to manage the performance concern for linting
+        // time. Our projects are not large enough to warrant this. Overrides AirBnB styles.
+        'import/no-cycle': 'off',


I believe we have inline exceptions to this rule, but I'll have to look it up

sounds like we can remove those inline exceptions then 🙌 . this is a good reminder that as we update the other sources for this config (item in the TODO) we should audit all the inline exceptions with a goal to remove as many as possible.

Last time I looked I didn't manage to remove the exceptions for this. We can take another spin this go around

.eslintrc.js

src/App.ts

stevengill · 2020-10-19T22:57:14Z

src/App.ts

-      (this.receiver as ExpressReceiver).installer !== undefined &&
-      (this.receiver as ExpressReceiver).installer!.authorize !== undefined
-    ) {
+    if ((this.receiver as ExpressReceiver).installer !== undefined


this looks way better IMO

thanks for the validation! humans: 1, prettier: 0. this takes a small manual effort and an eye for clarity, but i hope we all feel the result is worth it.

src/App.ts

stevengill · 2020-10-19T22:59:55Z

src/App.ts

@@ -380,10 +367,11 @@ export default class App {
    callbackIdOrConstraints: string | RegExp | Constraints,
    ...listeners: Middleware<SlackShortcutMiddlewareArgs<Extract<Shortcut, { type: Constraints['type'] }>>>[]
  ): void {
-    const constraints: ShortcutConstraints =
+    const constraints: ShortcutConstraints = (


I like that we are bringing back the round bracket to encapsulate the code below. I like this style better. Prettier removed them.

stevengill · 2020-10-19T23:11:06Z

src/App.ts

-        : type === IncomingEventType.Command
-        ? ((body as SlackCommandMiddlewareArgs['body']).user_id as string)
-        : undefined,
+  const teamId: string = (() => {


This refactor is much easier to read and follow. Good stuff!

i'm pretty happy that there are a lot less type assertions this way!

stevengill · 2020-10-20T18:48:34Z

src/App.ts

 import { Agent } from 'http';
 import { SecureContextOptions } from 'tls';
 import util from 'util';
 import { WebClient, ChatPostMessageArguments, addAppMetadata, WebClientOptions } from '@slack/web-api';
 import { Logger, LogLevel, ConsoleLogger } from '@slack/logger';
 import axios, { AxiosInstance } from 'axios';
+import allSettled from 'promise.allsettled';


I also want to add that I saw issues with removing require from allSettled last time too, but I don't remember what that issue was off the top of my head

seratch · 2020-10-20T21:41:43Z

src/WebClientPool.ts

@@ -0,0 +1,15 @@
+import { WebClient, WebClientOptions } from '@slack/web-api';


I like this idea but how about doing this in another (smaller) pull request?

its related to getting the lint rules to pass. the rule is max-classes-per-file. we could do this in a separate PR if we want to land it before this PR. otherwise this PR will be blocked on failing its own lint.

Thanks for sharing the context.

we could do this in a separate PR if we want to land it before this PR

I think there is no blocker for it. If we separate this change, we can quickly review and merge it. Also, if the change has issues by any chance, it's much easier to revert.

Once we removed StringIndexed from SlackEvent types, many property lookups became typechecking errors. I borrowed this refactor mostly from a WIP PR to improve our style/linting in a proactive effort to redue conflicts when that PR lands. The only changes I needed to add were the Org Apps changes that had landed on main. slackapi#669 In a few cases, the event types had other issues that became apparent only after removing the aggressive casting we had before. Those are fixed. It's now evident that there's more type related problems with respect to the Authorize, its related types, especially the OrgApps variants. I'll try to fix these next. It's also not clear if buildSource needs the orgInstall argument. Will seek advice in code review for that.

CLAassistant · 2021-02-11T04:52:56Z

All committers have signed the CLA.

seratch · 2021-03-23T01:39:31Z