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

Cannot access the user id in the "message_changed" event with TypeScript #2026

Open
okovpashko opened this issue May 12, 2023 · 4 comments
Open
Labels
area:typescript issues that specifically impact using the package from typescript projects bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented enhancement M-T: A feature request for new functionality pkg:types applies to `@slack/types` semver:major

Comments

@okovpashko
Copy link

okovpashko commented May 12, 2023

Reproducible in:

The Slack SDK version

3.13.1

Node.js runtime version

v16.15.1

OS info

ProductName: macOS
ProductVersion: 12.6.5
BuildVersion: 21G531
Darwin Kernel Version 21.6.0: Thu Mar 9 20:08:59 PST 2023; root:xnu-8020.240.18.700.8~1/RELEASE_X86_64

Steps to reproduce:

The Slack docs say that there should be the message.user key in the message_changed event containing the Slack user id.

The TypeScript interface for the MessageChangedEvent refers to the MessageEvent interface in the message and the previous_message properties that make TypeScript assume that accessing event.message.user is not allowed because not every subtype of the message event contains a user id.

It seems like the reference to the MessageEvent is incorrect because (I suppose) not every message can be edited and moreover it makes the circular dependency in types when the message_changed event can reference to the message_changed event and so on.

Expected result:

  • the message property of the message_changed event has the valid type according to the Slack docs
  • no TypeScript errors when accessing event.message.user for the message_changed event.

Actual result:

TS2339: Property 'user' does not exist on type 'MessageEvent'.
@seratch seratch added bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented and removed untriaged labels May 12, 2023
@seratch seratch self-assigned this May 12, 2023
@seratch
Copy link
Member

seratch commented May 12, 2023

Hi @okovpashko, thanks for taking the time to report this issue with details!

I agree that the message/ previous_message in message_changed subtype message event type definition should be corrected as you pointed out. We will resolve it in the next patch version. If you're happy to make a pull request for it, I'd love to have your contribution 🙌

@okovpashko
Copy link
Author

Hi @seratch
Thank you for the rapid response!

I will be happy to submit a PR with the fix. The only thing I need to understand beforehand is if there's already an existing type for the object I need or I should introduce a new interface for that. Could you give me a hint please?

@seratch
Copy link
Member

seratch commented May 12, 2023

The MessageEvent is a union type that consists of all the possible message event types, so we can define a new union type with less subtype patterns. I think removing at least message_changed, message_deleted should be safe enough. But, for the rest, I cannot confidently say anything without thorough tests with real payloads.

@filmaj filmaj mentioned this issue Jun 14, 2024
7 tasks
@filmaj filmaj transferred this issue from slackapi/bolt-js Sep 15, 2024
@filmaj filmaj added this to the [email protected] milestone Sep 15, 2024
@filmaj filmaj added semver:major enhancement M-T: A feature request for new functionality area:typescript issues that specifically impact using the package from typescript projects pkg:types applies to `@slack/types` labels Sep 15, 2024
@filmaj
Copy link
Contributor

filmaj commented Sep 15, 2024

Transferred this from the bolt-js repo, as now event payloads live in this package.

I think this issue is similar to #2025.

Definitely one way to fix this would be to narrow the type of the message on the MessageChangedEvent to reflect the more specific kinds of messages. That could be an initial improvement that could be a minor version upgrade.

However, I think another, perhaps more complete solution would be to use generics (type parameters) to be able to explicitly say what the base message is, e.g. MessageChangedEvent<Message extends EditableMessage = EditableMessage>. This way, developers defining handlers using bolt-js could then specifically type the event payload incoming, if applicable. Bolt could use some generally-applicable union of message types as the default (EditableMessage in the example above - which could leverage the minor-version improvement discussed in the previous paragraph), but we give developers the options to qualify it as they wish.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:typescript issues that specifically impact using the package from typescript projects bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented enhancement M-T: A feature request for new functionality pkg:types applies to `@slack/types` semver:major
Projects
None yet
Development

No branches or pull requests

3 participants