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

Draft: Improve types #1228

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Draft: Improve types #1228

wants to merge 3 commits into from

Conversation

raycharius
Copy link
Contributor

@raycharius raycharius commented May 7, 2021

Summary

Draft pull request to discussion in this issue around improving @slack/types. See discussion here.

Requirements (place an x in each [ ])

@gitwave gitwave bot added the untriaged label May 7, 2021
export interface Dialog {
title: string;
callback_id: string;
elements: {
type: 'text' | 'textarea' | 'select';
Copy link
Member

@seratch seratch May 10, 2021

Choose a reason for hiding this comment

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

I'm afraid that changing union type to enum may not be fully backward-compatible. I cannot tell what the possible troubles are by this but at least, unions and enums are not the same. To me, just naming the union types would be a safer refactoring. 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.

Just my two cents, but I really love enums more than unions aha

@seratch seratch added area:typescript issues that specifically impact using the package from typescript projects enhancement M-T: A feature request for new functionality pkg:types applies to `@slack/types` and removed untriaged labels May 11, 2021
@raycharius
Copy link
Contributor Author

I've been playing around with this a bit, and you are totally right. For some reason, I always though of enums as a subset of union types, but it will not work with simple strings.

Naming the union types would definitely make the code more structured, but that would be a change that's more internal for your team as opposed to offering value for developers using the SDK.

There is one other possibility, to have those values available as imports – export an object literal instead of an enum and create union types that reference the values in those objects. Advantage would be having those available as constants instead of working with strings for those using the package, but not sure if that's in scope.

@raycharius
Copy link
Contributor Author

What are thoughts on the discriminating union types for more strict type checking?

Advantages:

  • A much better and more type-safe experience with the package and SDK, such as with the WebClient.

Disadvantages:

  • Would require more swift updates to the package if API validation changes.
  • It could be a breaking change, depending on how breaking is defined. It could throw compiler errors for objects that don't comply with the API validation, but unless there are some cases that I'm not taking into consideration, it would readonly only be showing issues with current code that would probably be rejected by the API anyway.

@filmaj filmaj modified the milestones: types@3.x, types@3.0 Sep 26, 2023
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 enhancement M-T: A feature request for new functionality pkg:types applies to `@slack/types` semver:major
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants