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

MessageEvent incorrectly references the Node global MessageEvent type. #2020 #2021

Merged
merged 2 commits into from
Sep 10, 2024

Conversation

varmil
Copy link
Contributor

@varmil varmil commented Sep 10, 2024

Summary

This pull request resolves #2020

Requirements (place an x in each [ ])

Copy link

Thanks for the contribution! Before we can merge this, we need @varmil to sign the Salesforce Inc. Contributor License Agreement.

@seratch
Copy link
Member

seratch commented Sep 10, 2024

@varmil Could you sign your CLA (see the above bot message)? Without that, we are unable to accept any external contributions 🙇

@seratch seratch added bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented pkg:types applies to `@slack/types` labels Sep 10, 2024
@seratch seratch requested a review from filmaj September 10, 2024 08:54
Copy link

codecov bot commented Sep 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.63%. Comparing base (a7cc255) to head (d270fe6).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2021   +/-   ##
=======================================
  Coverage   91.63%   91.63%           
=======================================
  Files          37       37           
  Lines        9946     9946           
  Branches      633      633           
=======================================
  Hits         9114     9114           
  Misses        820      820           
  Partials       12       12           
Flag Coverage Δ
cli-hooks 95.23% <ø> (ø)
cli-test 95.39% <ø> (ø)
oauth 77.39% <ø> (ø)
socket-mode 58.22% <ø> (ø)
web-api 96.87% <ø> (ø)
webhook 96.65% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@varmil
Copy link
Contributor Author

varmil commented Sep 10, 2024

Thank you for your prompt confirmation. I have completed the CLA. Is this okay?

@seratch
Copy link
Member

seratch commented Sep 10, 2024

Yes, good to go! Other members in my team will be checking this PR soon. Thanks again for taking the time to fix this!

@filmaj
Copy link
Contributor

filmaj commented Sep 10, 2024

Took me a bit to understand why this solution works, but it seems like references to MessageEvent inside message.ts refer to the built-in node MessageEvent interface and not the Slack one:

Screenshot 2024-09-10 at 11 06 11 AM

I will look to add a type test in a follow-up PR to catch this. Thank you!

@filmaj filmaj merged commit beb0345 into slackapi:main Sep 10, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented cla:signed pkg:types applies to `@slack/types`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MessageEvent incorrectly references the Node global MessageEvent type.
3 participants