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

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

Merged
merged 8 commits into from
Dec 16, 2019

Conversation

tteltrab
Copy link
Contributor

@tteltrab tteltrab commented Dec 11, 2019

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.

image

Left comments to further elaborate.

Requirements (place an x in each [ ])

src/App.ts Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Dec 11, 2019

Codecov Report

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

Impacted file tree graph

@@            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 stevengill self-assigned this Dec 12, 2019
Copy link
Member

@stevengill stevengill left a comment

Choose a reason for hiding this comment

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

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

@tteltrab
Copy link
Contributor Author

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

@stevengill
Copy link
Member

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

src/App.ts Show resolved Hide resolved
@@ -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)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@tteltrab tteltrab Dec 13, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@aoberoi aoberoi Dec 13, 2019

Choose a reason for hiding this comment

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

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?

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 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.

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

Choose a reason for hiding this comment

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

ditto to previous comment.

Copy link
Contributor Author

@tteltrab tteltrab Dec 13, 2019

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@tteltrab tteltrab Dec 15, 2019

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
@tteltrab
Copy link
Contributor Author

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
Copy link
Contributor

aoberoi commented Dec 13, 2019

@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 Show resolved Hide resolved
package.json Show resolved Hide resolved
@tteltrab tteltrab changed the title 🧹 low-hanging ts lint fixes 🧹 mostly low-hanging ts lint fixes, ts lint as part of test Dec 15, 2019
Copy link
Contributor

@aoberoi aoberoi left a comment

Choose a reason for hiding this comment

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

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

src/conversation-store.ts Show resolved Hide resolved
@aoberoi aoberoi merged commit 252aaf2 into slackapi:master Dec 16, 2019
@tteltrab tteltrab deleted the ts-fixes branch December 16, 2019 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants