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

types: export helper union types #1819

Merged
merged 4 commits into from
Jun 14, 2024
Merged

types: export helper union types #1819

merged 4 commits into from
Jun 14, 2024

Conversation

filmaj
Copy link
Contributor

@filmaj filmaj commented Jun 13, 2024

Summary

This PR fixes #1818. It exports a bunch of helper unions that we use when authoring these types, but that we've heard from the community that they could possibly want to use themselves in their apps (see #1227).

@filmaj filmaj added semver:minor area:typescript issues that specifically impact using the package from typescript projects pkg:types applies to `@slack/types` labels Jun 13, 2024
@filmaj filmaj added this to the [email protected] milestone Jun 13, 2024
@filmaj filmaj self-assigned this Jun 13, 2024
Copy link

codecov bot commented Jun 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.76%. Comparing base (1583183) to head (1ba295a).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1819   +/-   ##
=======================================
  Coverage   81.76%   81.76%           
=======================================
  Files          35       35           
  Lines        7742     7742           
  Branches      316      316           
=======================================
  Hits         6330     6330           
  Misses       1400     1400           
  Partials       12       12           
Flag Coverage Δ
cli-hooks 95.07% <ø> (ø)
cli-test 53.80% <ø> (ø)
oauth 76.51% <ø> (ø)
socket-mode 59.41% <ø> (ø)
web-api 96.53% <ø> (ø)
webhook 95.20% <ø> (ø)

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

@filmaj filmaj requested a review from a team June 13, 2024 21:14
Copy link
Member

@zimeg zimeg left a comment

Choose a reason for hiding this comment

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

Solid improvements as always! Having exposed types for rich text is super helpful, so thank you for typing this 👏

All of these changes look good to me and the JSDoc is awesome 🚀 📚 No blockers but for my own understanding, are we wanting to export types in this granular way in most cases?

Comment on lines +247 to +248
export type SectionBlockAccessory = Button | Checkboxes | Datepicker | ImageElement | MultiSelect | Overflow |
RadioButtons | Select | Timepicker | WorkflowButton;
Copy link
Member

Choose a reason for hiding this comment

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

Great to see these ordered and matching the docs 🙌

@filmaj
Copy link
Contributor Author

filmaj commented Jun 14, 2024

are we wanting to export types in this granular way in most cases?

@zimeg Great question; I don't think so, and I would personally default to keeping things internal/non-exported, at least as an initial move, because:

  1. It's easier to put something out there than it is to retract; adding something new is additive while taking something away is a breaking change.
  2. In terms of TS specifically, it can be harder to know and manage what is unused if we just export everything.
  3. In this particular case, we specifically had a request from the community to export this particular type; see this OP in Add Timepicker to @slack/types #1226:

Moving union types to their own exported types. For example, elements supported within the ActionsBlock having a type of ActionsElements. For SectionBlock, SectionElements. Etc.

Of course always open to discussing and changing this inclination if the team feels otherwise.

@filmaj filmaj merged commit f8d06ca into main Jun 14, 2024
21 checks passed
@filmaj filmaj deleted the types-additional-unions branch June 14, 2024 04:44
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 pkg:types applies to `@slack/types` semver:minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

types: export additional union types used internally
3 participants