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

Fix update image block type #1738

Closed
wants to merge 9 commits into from

Conversation

cfeeney5
Copy link
Contributor

Summary

This update adds the newly available slack_file object to the imageBlock and sets the image_url to be optional

Requirements (place an x in each [ ])

This update adds the newly available slack_file object to the imageBlock and sets the image_url to be optional
Copy link

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

@seratch seratch added area:typescript issues that specifically impact using the package from typescript projects pkg:types applies to `@slack/types` labels Jan 29, 2024
@seratch
Copy link
Member

seratch commented Jan 29, 2024

Thank you for taking the time to send this! As I mentioned at #1734 (comment), a similar change is required on the image block element side. Also, we prefer union types for this.

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.

Thanks for sending a PR for this! Much appreciated.

However, we will require a few changes:

  1. We should factor the Slack File object into its own type, and, because it is documented as a 'composition object' on our API site, it should belong in the same area of the types project: composition-objects.ts. We also want to make sure to give it a top-level JSDoc, with links to the API - just like the other objects in this source file. If you could please follow that style, that would be great!
  2. There is a subtle detail in the Slack File composition object that is described in the docs:

This file must be an image and you must provide either the URL or ID. In addition, the user posting these blocks must have access to this file. If both are provided then the payload will be rejected.

So either url or id must be provided - but not both. Similarly, either image_url or slack_file must be provided - but not both. We should express this in TypeScript, using the union type, as @seratch mentioned.

So both the slack_file object should itself be a union type, and the ImageBlock (and ImageElement that once more Kaz pointed out also needs to be updated in the same way), need to express this either-or constraint.

Something like (with, of course, nice JSDocs and links to our API site, etc.):

interface SlackFileViaURL = {
  url: string;
}
interface SlackFileViaID = {
  id: string;
}
type SlackFile = SlackFileViaURL | SlackFileViaID;
interface ImageElementWithImageURL = {
  image_url: string;
}
interface ImageElementWithSlackFile = {
  slack_file: SlackFile;
}
type ImageElement = {
  type: 'image';
} & (ImageElementWithImageURL | ImageElementWithSlackFile);

The benefit to this approach is that the required properties are now properly expressed as required properties and we are able to fully model the exclusive-or constraint on these types.

…ImageBlock and ImageElement

The new slack_file object must contain and id or a url so we use a union type to ensure either id or url is used.
ImageBlock and ImageElement have also been updated. ImageBlock has been changed to a type so a union can be used
@cfeeney5
Copy link
Contributor Author

Ive refactored this now for use with unions. UrlImageObject and SlackFileImageObject are exported from block-elements so it can be used there and in blocks. Im not sure which way is preferred.

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.

Nice, getting closer! Left some more suggestions and a few (hopefully final) change requests.

packages/types/src/block-kit/composition-objects.ts Outdated Show resolved Hide resolved
packages/types/src/block-kit/composition-objects.ts Outdated Show resolved Hide resolved
packages/types/src/block-kit/composition-objects.ts Outdated Show resolved Hide resolved
packages/types/src/block-kit/composition-objects.ts Outdated Show resolved Hide resolved
packages/types/src/block-kit/composition-objects.ts Outdated Show resolved Hide resolved
packages/types/src/block-kit/block-elements.ts Outdated Show resolved Hide resolved
packages/types/src/block-kit/block-elements.ts Outdated Show resolved Hide resolved
packages/types/src/block-kit/blocks.ts Outdated Show resolved Hide resolved
packages/types/src/block-kit/blocks.ts Outdated Show resolved Hide resolved
@filmaj
Copy link
Contributor

filmaj commented Jan 30, 2024

I've enabled the CI tests for this PR and it seems the linter is now failing. Could you please also fix those up? I think many of the formatting changes you introduced will not be allowed according to our linter configuration / style guide.

@filmaj
Copy link
Contributor

filmaj commented May 1, 2024

#1783 has superseded this PR; closing.

@filmaj
Copy link
Contributor

filmaj commented Jun 5, 2024

FYI the fix for this has been released to npm as @slack/types v2.12.0.

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 cla:signed pkg:types applies to `@slack/types`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants