🧹 mostly low-hanging ts lint fixes, ts lint as part of test #339

tteltrab · 2019-12-11T23:21:39Z

Summary

Some simple typescript lint fixes. Understand the move might be to eslint at some point, but these still seem like they won't hurt anyone.

Fixes all of these errors but the conversion-store promise one.

Left comments to further elaborate.

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.

src/App.ts

codecov · 2019-12-11T23:28:42Z

Codecov Report

Merging #339 into master will decrease coverage by 0.35%.
The diff coverage is 79.31%.

@@            Coverage Diff             @@
##           master     #339      +/-   ##
==========================================
- Coverage   73.71%   73.35%   -0.36%     
==========================================
  Files           7        7              
  Lines         506      503       -3     
  Branches      145      146       +1     
==========================================
- Hits          373      369       -4     
- Misses        101      102       +1     
  Partials       32       32

Impacted Files	Coverage Δ
src/conversation-store.ts	`96.29% <0%> (-3.71%)`	⬇️
src/App.ts	`79.19% <100%> (ø)`	⬆️
src/helpers.ts	`92.3% <50%> (ø)`	⬆️
src/ExpressReceiver.ts	`50.96% <80.95%> (-1.82%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9723d11...56994bd. Read the comment docs.

stevengill

These changes look good to me. I don't see any logic changes. Only one lint failure is remaining after these fixes.

tteltrab · 2019-12-12T18:55:39Z

@stevengill - thanks for the review 👍 any thoughts on the coverage tool failing? I didn't change any logic, and tests are still passing.

stevengill · 2019-12-12T18:57:21Z

@tteltrab We need to fix up our coverage failures. Nothing to do with your PR.

src/App.ts

aoberoi · 2019-12-13T01:40:45Z

src/ExpressReceiver.ts

@@ -134,7 +134,7 @@ export default class ExpressReceiver extends EventEmitter implements Receiver {
    return new Promise((resolve, reject) => {
      // TODO: what about synchronous errors?
      this.server.close((error) => {
-        if (error) {
+        if (Boolean(error)) {


whoa, def do not want to be boxing these values in objects. since error is typed to Error | undefined, i think its better to change this condition to error !== undefined.

I agree error !== undefined is better. Out of curiosity, isn't Boolean(error) what the code was doing before, just explicitly instead of implicitly?

i learned something new today! when i quickly read over this before, i thought we were boxing boolean primitives in Boolean objects (new Boolean()), which behave in confusingly different ways. in actuality, this was Boolean() used as a function, which does something different.

I found a small hint about this in the MDN docs for logical operators:

The same conversion can be done through the Boolean function.

Double NOT is essentially coercing a value to a boolean primitive using the "truthiness" rules in JavaScript. the original code here was intended to evaluate the truthiness of error, so the Boolean function makes a lot of sense.

The tslint rule which makes the original code fail is on because depending on the truthiness is a common pitfall in JavaScript. It's not immediately clear which values are and are not going to evaluate to the "true" branch. But I don't see how using the Double NOT (or the Boolean function) is helpful in making it more clear.

I think we have to decide if we find value in that rule or not. If we want to keep it, then we should explicitly check for the values that we care about instead of using truthiness. If we don't want to keep it, then using the Boolean function seems like a reasonable choice.

There's also a number of options to the tslint rule to consider. The most relevant one to us would be allow-undefined-union, which if we turned on should allow Error | undefined in conditionals without an error.

Thoughts?

I think it ended up catching some interesting cases here, and led to better code, so probably worth keeping? Avoiding checking truthiness is probably for the best.

aoberoi · 2019-12-13T01:41:37Z

src/ExpressReceiver.ts

@@ -218,7 +218,7 @@ async function verifyRequestSignature(
  body: string,
  signature: string,
  requestTimestamp: number): Promise<void> {
-  if (!signature || !requestTimestamp) {
+  if (!Boolean(signature) || !Boolean(requestTimestamp)) {


ditto to previous comment.

I think somewhere in your code you're relying on the implicit Boolean conversion of NaN for requestTimestamp (at least in the tests? I believe that's why they're failing now after the change).

Also !== undefined does a different thing logically, which I'm not sure is wanted here?

You're converting the x-slack-request-timestamp header into a number here: https://github.com/slackapi/bolt/blob/master/src/ExpressReceiver.ts#L191

And then, in the spec, you're verifying that it detects that missing header: https://github.com/slackapi/bolt/blob/master/src/ExpressReceiver.spec.ts#L111

So if you compare requestTimestamp === undefined it won't work, but maybe fine to use isNaN(requestTimestamp) since it's assumed the type of the input is a number? Updated to that.

Should L190-L191 look like the following?

const signature = req.headers['x-slack-signature'] as string | undefined; const ts = Number(req.headers['x-slack-request-timestamp']) as number | NaN;

Then the verifyRequestSignature() function's arguments would change type:

signature: string -> signature: string | undefined

requestTimestamp: number -> requestTimestamp: number | NaN

If we did this, the verifyRequestSignature() function would be more resilient in terms of how it can be used, especially if we end up refactoring it into another package as the comment above suggests.

However, I don't quite think NaN was ever intended to be an acceptable value for requestTimestamp. That just feels like a mistake on L191. I think the intention would be for this argument's type to be number | undefined. I'd personally rather see this change. It would also be more resilient than today's implementation.

NaN is a value (not a type - typeof NaN === 'number'), so I don't think as number | NaN would work for L191 ?

My thinking around checking if it's NaN - isNaN(undefined) is false anyway. Agree number | undefined is probably more the intention/semantically what you were originally trying to handle, but isNaN handles that case plus the case that someone sends something like "Hey there!" as the timestamp, so the timestamp is defined but in trying to convert the string to a number you get NaN, which this would catch.

I'll take a stab at this - I think the thing that makes sense is to have the function that verifies the request signature get the header as is and do the number conversion/check, since there's a couple things you want to check for there. I can take a stab at this and see how it looks.

Ended up just thinking it's probably better to check for the two cases explicitly - either it's undefined, or it's invalid in a couple of different ways (not a number, too old). So just broke it out into two checks.

src/ExpressReceiver.ts

tteltrab · 2019-12-13T14:53:11Z

Updated based on review comments. Also added a (seemingly missing?) test to check that it errors when the signature is missing, not just when then timestamp is missing.

aoberoi · 2019-12-13T20:49:10Z

@tteltrab Do you think we should use npm run lint && as a prefix to the current "test" script in package.json so that we get linting run in CI?

package.json

src/ExpressReceiver.spec.ts

src/ExpressReceiver.ts

src/conversation-store.ts

aoberoi

this is fantastic! thanks for sticking with us and driving these changes @tteltrab 🍾

src/conversation-store.ts

low-hanging ts lint fixes

162ed1f

tteltrab commented Dec 11, 2019

View reviewed changes

src/App.ts Show resolved Hide resolved

stevengill added the semver:patch label Dec 12, 2019

stevengill self-assigned this Dec 12, 2019

stevengill approved these changes Dec 12, 2019

View reviewed changes

aoberoi suggested changes Dec 13, 2019

View reviewed changes

Nick Bartlett added 3 commits December 13, 2019 09:05

check error undefined, better formatting, parseRequestBody returns any

6120711

add back in accidentally reverted semicolon

9489a81

check isNaN for timestamp, add seemingly missing test

08644e3

Nick Bartlett added 4 commits December 15, 2019 00:41

explicitly test invalid timestamp header

d7f3030

📝 fix typo in test

e196e7a

👕 handle final lint error, catch after then(next)

02d8826

lint as part of test script

56994bd