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

Update code to fix TS v.4.4 compiler errors #1091

Merged
merged 12 commits into from
Aug 27, 2021
Merged

Conversation

srajiang
Copy link
Member

Summary

Fix #1089 by updating code which causes Typescript v4.4.1 compiler errors.

Requirements (place an x in each [ ])

@gitwave gitwave bot added the untriaged label Aug 27, 2021
@codecov
Copy link

codecov bot commented Aug 27, 2021

Codecov Report

Merging #1091 (fef4dc2) into main (bcca0ee) will increase coverage by 0.13%.
The diff coverage is 38.09%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1091      +/-   ##
==========================================
+ Coverage   70.90%   71.03%   +0.13%     
==========================================
  Files          13       13              
  Lines        1220     1229       +9     
  Branches      365      365              
==========================================
+ Hits          865      873       +8     
- Misses        284      287       +3     
+ Partials       71       69       -2     
Impacted Files Coverage Δ
src/receivers/HTTPReceiver.ts 32.40% <0.00%> (+1.94%) ⬆️
src/receivers/SocketModeReceiver.ts 68.42% <0.00%> (-1.23%) ⬇️
src/App.ts 83.28% <100.00%> (+0.09%) ⬆️
src/conversation-store.ts 100.00% <100.00%> (ø)

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 bcca0ee...fef4dc2. Read the comment docs.

@srajiang srajiang self-assigned this Aug 27, 2021
@srajiang srajiang marked this pull request as ready for review August 27, 2021 04:30
@srajiang srajiang added TypeScript-specific tests M-T: Testing work only labels Aug 27, 2021
@srajiang
Copy link
Member Author

Casting error to type any has added additional uncovered lines. I will take a look more tomorrow am at this!

@seratch seratch added this to the 3.7.0 milestone Aug 27, 2021
@seratch seratch changed the title Update code to fix TS v.4.4.1 compiler errors Update code to fix TS v.4.4 compiler errors Aug 27, 2021
Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

This looks good to me!

Perhaps any additional coverage improvements could be worked on in a separate PR in order to move ahead with merging this? This way the other PRs waiting on this project can move forward? The ESLint config update PR, for example, is itself a blocker to merging two other open PRs.

EDIT: Woops, didn't notice that the coverage diff GitHub Action check is actually blocking merging this PR - I'll see if I can add some tests to help that.

@seratch
Copy link
Member

seratch commented Aug 27, 2021

@filmaj

EDIT: Woops, didn't notice that the coverage diff GitHub Action check is actually blocking merging this PR - I'll see if I can add some tests to help that.

Oh, but I think that we don't need to be too strict about it. Particularly, for this pull request, we can consider merging it as early as possible not to block other pull requests.

@filmaj
Copy link
Contributor

filmaj commented Aug 27, 2021

I've opened #1094 adding an additional test for HTTPReceiver that I issued with the intention of merging into this PR. I think it should help push code coverage up.

@filmaj
Copy link
Contributor

filmaj commented Aug 27, 2021

Oh geez, I 'merged' my test expansion not realizing that this PR is based off of @srajiang's fork 😆 😢 🙈

Anyways, the ts-4.4 branch that exists on this repo's origin has the combination of this PR's commits + my one commit with a new test. Perhaps that can help in the quest for expanded code coverage! Feel free to merge into your PR if you wish, @srajiang.

@srajiang
Copy link
Member Author

I've merged @filmaj your additional commit with the additional test, thanks for that. Thanks for weighing in @seratch - I'm going to merge this now despite the failing codecov/patch test.

@srajiang srajiang merged commit e23b99c into slackapi:main Aug 27, 2021
@srajiang srajiang deleted the ts-4.4 branch August 27, 2021 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests M-T: Testing work only TypeScript-specific
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compilation errors with TypeScript v4.4 in CI builds
3 participants