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.

Sign up for GitHub

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

Allow mrkdwn_in, callback_id fields in message attachments #528

Merged
merged 6 commits into from
Apr 13, 2018
Merged

Allow mrkdwn_in, callback_id fields in message attachments #528

Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
301d00b
Merge pull request #2 from slackapi/master
DominikPalo Feb 7, 2018
e93c377
Merge pull request #3 from slackapi/master
DominikPalo Apr 11, 2018
12b6f1a
Allow the mrkdwn_in field in message attachments
DominikPalo Apr 11, 2018
f506855
Allow the attachment_type and the callback_id fields in message attac…
DominikPalo Apr 11, 2018
c9bf842
Fix typo
DominikPalo Apr 12, 2018
951fe41
Remove the unnecessary field 'attachment_type'
DominikPalo Apr 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/methods.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,9 @@ export interface MessageAttachment {
type: string;
text?: string;
}[];
attachment_type?: 'default';
Copy link
Contributor

Choose a reason for hiding this comment

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

while i see that this is an accepted field, there are no values other than 'default', so i don't think it adds any value to type it (i'm not sure there will ever be any other possible values, and each additional property has a visual cost when your editor is trying to give you hints)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, I understand, but our UX team uses Walkiebot for designing flows and messages. We also plan to fetch those messages designed by Walkiebot automatically to our integrations (using the Walkiebot API), but there is a problem - Walkiebot generates JSON/JS of designed attachments with attachmet_type field included (see the attached screenshot), therefore the output is not compatible with this SDK and we have to manually remove this field from every exported message. I can contact Walkiebot and ask them to remove this field, but it seems, they strictly stick to Slack API specs, so maybe as a first step is good to remove this unnecessary field from Slack API documentation.
walkiebot

Copy link
Contributor

Choose a reason for hiding this comment

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

oh that's really cool! i'd love to make sure there's a great experience when integrating with Walkiebot, we recommend their product to loads of designers/developers.

what kind of error are you getting when you have the additional property in your API call? if you're in a javascript project, the client shouldn't care about additional properties. if you're in a typescript project, i thought our addition of the AuxiliaryArguments type would make this usage valid as well (interesting use case for the discussion @ #496)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, we use TypeScript for all our projects - but I thought before that our attachments were incompatible with the MessageAttachment because of missing the attachment_type field, but as I see now, my assumption is not true (thanks to the AuxiliaryArguments). Our incompatibility comes from missing types in the nested field MessageAttachment.actions - the current type definition for actions contains only two fields:

actions?: {
    type: string;
    text?: string;
  }[];

but the attachment actions in our project contains additional fields. So I will prepare a separate PR also with complete definitions for the attachment actions.

callback_id?: string;
mrkdwn_in?: ('pretext' | 'text' | 'fields')[];
Copy link
Contributor

Choose a reason for hiding this comment

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

this works and its right, but i was a little confused by the syntax. would you mind making a named string-based enum type and then setting this type to an array of that?

Copy link
Contributor Author

@DominikPalo DominikPalo Apr 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I would prefer to use a union type instead of an enum, because if a declare it as an enum type, e.g.:

export enum MrkdwnIn {
  Pretext: 'pretext',
  Text: 'text',
  Fields: 'fields'
}

MessageAttachment {
  //...
  mrkdwn_in?: MrkdwnIn[];
}

it won't be possible to define mesages in this "plain" way

const attachment: MessageAttachment = {
  mrkdwn_in: ['pretext']
}

and forces a user to use this style:

const attachment: MessageAttachment = {
  mrkdwn_in: ['MrkdownIn.Pretext']
}

so it won't be compatible with exports from tools like Walkiebot (and also with old projects, based on the older SDK).

But I can write it as a separate union type instead, to make it more readable:

export type MrkdwnIn = 'pretext' | 'text' | 'fields';

MessageAttachment {
  //...
  mrkdwn_in?: MrkdwnIn[];
}

what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

😮 i guess i never really gave any thought to the intricate differences between a union type and an enum. i always thought that enums were a "more powerful" union type because TypeScript would generate a symbol with the actual map, but didn't realize the compiler would complain when you use the literal values like in your example. i tried it out here.

in light of this fact, i agree that the union type is a better solution. thanks!

}

export interface LinkUnfurls {
Expand Down