ESLint Config Migration: Disable the only occurrences of no-nested-ternary rule violation by filmaj · Pull Request #1055 · slackapi/bolt-js

filmaj · 2021-08-09T21:28:27Z

Summary

This is a PR that should be merged into #1024 and incrementally addresses #842.

This PR disables the no-nested-ternary rule in the three occurrences that it happens - but is this the right approach? The section where the eslint-disable lines I added is particularly gnarly 😞 . I think the intention behind the no-nested-ternary rule is noble, case in point, the nested ternaries are super hard to read!

What do people think? Perhaps it is worth re-writing this section of the code, expanding it a little, so as to make it a bit more readable? Perhaps with a switch statement?

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.

seratch · 2021-08-09T21:43:19Z

What do people think? Perhaps it is worth re-writing this section of the code, expanding it a little, so as to make it a bit more readable? Perhaps with a switch statement?

I think that it's okay to have a few eslint-disable-line comments for now if we have a TODO/FIXME comment there. But, in final shape, I like the approach to rewrite this part using switch statements. If you do the rewriting in this PR, that's also fine!

…ight approach?

seratch

I approve this PR if we go with the comments this time

misscoded

What do people think? Perhaps it is worth re-writing this section of the code, expanding it a little, so as to make it a bit more readable? Perhaps with a switch statement?

I second the rewriting of this section at some point to make it more readable

filmaj · 2021-08-10T14:10:53Z

src/App.ts

-    // Set body and payload (this value will eventually conform to AnyMiddlewareArgs)
+    // Set body and payload
+    // TODO: this value should eventually conform to AnyMiddlewareArgs
+    let payload: any = {};


I didn't spend much time trying to figure out the various possible types the payload variable could end up being, so I took the lazy way and set this as any. Sounds like it is related to the TODO comment above? Let me know, happy to dig in here further if the team thinks it is worthwhile.

Trying to use a union type like DialogSubmitAction | WorkflowStepEdit | SlackShortcut | KnownEventFromType<string> | ... may make this safer. If we go with any here, we will lose the type safety at the listenerArgs initialization below.

filmaj · 2021-08-10T14:12:50Z

src/App.ts

+        payload = (bodyArg as SlackShortcutMiddlewareArgs['body']);
+        break;
+      // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+      // @ts-ignore: Fallthrough case in switch


We have noFallthroughCasesInSwitch enabled in tsconfig.json, which I think is a sensible default. However, I think this is one situation where a fallthrough in a case in a switch actually works to preserve the logic.

What does everyone think? Too complicated? Too many linter / compiler ignore comments to go along with it?

// If above conditional does not hit, fall through to fallback payload in default block below

This is reasonable enough, so that I'm happy to ignore the rule here.

filmaj · 2021-08-10T14:14:58Z

I added a commit to this PR attempting to rewrite the ternary operators to a switch statement. It has its own.. special gotchas, in its own way, so another quick review would be much appreciated 🙏

If it's too complicated / not clear / deemed not an improvement, no problem to drop the last commit, or try a different approach, or address at a later time - just let me know!

seratch

Perhaps, we still can explore better ways to ensure the type safety of payload in the code

seratch · 2021-08-11T00:12:25Z

src/App.ts

-    // Set body and payload (this value will eventually conform to AnyMiddlewareArgs)
+    // Set body and payload
+    // TODO: this value should eventually conform to AnyMiddlewareArgs
+    let payload: any = {};


Trying to use a union type like DialogSubmitAction | WorkflowStepEdit | SlackShortcut | KnownEventFromType<string> | ... may make this safer. If we go with any here, we will lose the type safety at the listenerArgs initialization below.

seratch · 2021-08-11T00:20:50Z

src/App.ts

+        payload = (bodyArg as SlackShortcutMiddlewareArgs['body']);
+        break;
+      // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+      // @ts-ignore: Fallthrough case in switch


// If above conditional does not hit, fall through to fallback payload in default block below

This is reasonable enough, so that I'm happy to ignore the rule here.

filmaj · 2021-08-11T14:25:20Z

OK, did another pass to not use any type. 🤞

seratch

Thanks for fixing it!

seratch · 2021-08-11T21:47:49Z

src/types/events/index.ts

 */
-type EventFromType<T extends string> = KnownEventFromType<T> extends never ? BasicSlackEvent<T> : KnownEventFromType<T>;
-type KnownEventFromType<T extends string> = Extract<SlackEvent, { type: T }>;
+export type EventFromType<T extends string> = KnownEventFromType<T> extends never ?


Exporting these types should be fine.

filmaj added the tests M-T: Testing work only label Aug 9, 2021

filmaj requested review from misscoded, seratch and stevengill August 9, 2021 21:28

filmaj self-assigned this Aug 9, 2021

filmaj mentioned this pull request Aug 9, 2021

Moving fully to eslint, removing prettier #1024

Merged

26 tasks

Disable the only occurrences of no-nested-ternary - but is this the r…

09e48d0

…ight approach?

filmaj force-pushed the no-nested-ternary branch from d10db51 to 09e48d0 Compare August 10, 2021 00:40

seratch approved these changes Aug 10, 2021

View reviewed changes

misscoded approved these changes Aug 10, 2021

View reviewed changes

Attempt at rewriting nested ternary payload assignment into a switch.

0a348f3

filmaj commented Aug 10, 2021

View reviewed changes

filmaj requested review from misscoded and seratch August 10, 2021 14:13

seratch reviewed Aug 11, 2021

View reviewed changes

Specify type of payload when processing events

1b5a162

filmaj requested a review from seratch August 11, 2021 14:24

seratch approved these changes Aug 11, 2021

View reviewed changes

filmaj merged commit d51c85e into tslint-to-eslint Aug 12, 2021

filmaj deleted the no-nested-ternary branch August 12, 2021 01:15

Conversation

filmaj commented Aug 9, 2021

Summary

Requirements (place an x in each [ ])

Uh oh!

seratch commented Aug 9, 2021

Uh oh!

seratch left a comment

Choose a reason for hiding this comment

Uh oh!

misscoded left a comment

Choose a reason for hiding this comment

Uh oh!

filmaj Aug 10, 2021

Choose a reason for hiding this comment

Uh oh!

seratch Aug 11, 2021

Choose a reason for hiding this comment

Uh oh!

filmaj Aug 10, 2021

Choose a reason for hiding this comment

Uh oh!

seratch Aug 11, 2021

Choose a reason for hiding this comment

Uh oh!

filmaj commented Aug 10, 2021

Uh oh!

seratch left a comment

Choose a reason for hiding this comment

Uh oh!

seratch Aug 11, 2021

Choose a reason for hiding this comment

Uh oh!

seratch Aug 11, 2021

Choose a reason for hiding this comment

Uh oh!

filmaj commented Aug 11, 2021

Uh oh!

seratch left a comment

Choose a reason for hiding this comment

Uh oh!

seratch Aug 11, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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