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

Component enum property rule proposal #1568

Closed
2 of 3 tasks
Tracked by #1800
sungik-choi opened this issue Aug 28, 2023 · 2 comments · Fixed by #1990
Closed
2 of 3 tasks
Tracked by #1800

Component enum property rule proposal #1568

sungik-choi opened this issue Aug 28, 2023 · 2 comments · Fixed by #1990
Assignees
Labels
enhancement Issues or PR related to making existing features better

Comments

@sungik-choi
Copy link
Contributor

sungik-choi commented Aug 28, 2023

Description

컴포넌트의 enum 속성 컨벤션 변경을 제안합니다. enum을 모두 union string literal로 통일합니다.

Reasons for suggestion

enum의 문제

beizer-react에선 size, variants등의 값에 ***Size, ***Variants 같은 enum을 사용하고 있습니다. string literal을 사용할 수도 있지만, 소스 코드 검색 시에 enum이 가지는 이점이 있어 사용하게 되었습니다. 반면 enum이 장점만을 가지는 것은 아닙니다.

단점

  1. 컴포넌트와 별도로 export 되므로 사용처에서 size, variants 등의 값을 커스텀하자면 이를 모두 import 해야합니다
  2. size, variants 같은 속성이 늘어날 때마다 enum도 속성 수에 비례하여 늘어나야합니다
  3. IDE의 자동 완성으로 자동 import를 해준다고 해도, 소스 코드가 방대할 경우 자동 완성까지 지연 시간이 발생합니다. 팀 전체에서 이 지연 시간을 모은다면 꽤나 유의미한 수치가 될 거로 예상합니다
  4. 현재 어떤 속성이 enum이여야하는지/아닌지에 대한 기준이 불명확합니다. 예컨대, Stackjustifyalign 속성은 enum이 아닌 string literal로 되어있습니다. 명확한 기준을 정하기도 어렵습니다

기존 장점이었던 '소스 코드 검색의 용이성'은 특히 마이그레이션을 하는 경우 빛을 발하게 되는데, 이제는 codemod 스크립트를 통해 AST를 탐색하여 마이그레이션을 할 수 있습니다. 현 시점에서 이 장점을 유지하기 위한 비용보다 단점에서 오는 비용이 훨씬 더 크다고 생각합니다.

Proposed solution

enum을 모두 union (string or number) literal로 통일합니다.

// AS-IS
enum ButtonSize {
  XS = 'xs',
  S = 's',
}

enum ButtonVariants {
  Primary = 'primary',
  Secondary = 'secondary',
}
// TO-BE
type ButtonSize = 'xs' | 's'
type ButtonVariants = 'primary' | 'secondary'

마이그레이션 시 장점

  • 별도의 import 없이, 해당 속성값에 커서를 위치시키는 것만으로 자동 완성이 바로 가능
  • 별도의 캐스팅이 불필요함 (as ButtonSize 같은)
  • 코드 컨벤션 통일
  • enum과 달리 type은 코드를 생성하지 않으므로 번들 용량 감소

마이그레이션 시 단점

  • variants 이름이 겹치는 경우 단순 소스 코드 검색이 enum보다 어려움
  • enum은 특정값 deprecated 처리가 쉬움 (예: ButtonColorVariant.Monochrome)
  • 마이그레이션 비용 (codemod)

References

Tasks

Preview Give feedback
  1. bezier-codemod feat
    Dogdriip
  2. enhancement
    Dogdriip
@sungik-choi
Copy link
Contributor Author

sungik-choi commented Aug 28, 2023

마이그레이션 전략

  • stable(1.y.z) 버전에서는 enum을 deprecated 처리하고, enum과 union literal type 둘 다 지원
  • 채널 데스크 애플리케이션의 마이그레이션은 진행
  • 다음 메이저 버전에서 enum 지원 중단

@sungik-choi
Copy link
Contributor Author

@Dogdriip 이 이슈 + 마이그레이션 코드 구현 및 문서 작성해보시면 좋을 거 같아요!

@sungik-choi sungik-choi added the enhancement Issues or PR related to making existing features better label Sep 4, 2023
@github-project-automation github-project-automation bot moved this to 📌 Backlog in Bezier React Sep 4, 2023
sungik-choi pushed a commit that referenced this issue Sep 14, 2023
…eprecate enum (#1595)

<!--
  How to write a good PR title:
- Follow [the Conventional Commits
specification](https://www.conventionalcommits.org/en/v1.0.0/).
  - Give as much context as necessary and as little as possible
  - Prefix it with [WIP] while it’s a work in progress
-->

## Self Checklist

- [x] I wrote a PR title in **English**.
- [x] I added an appropriate **label** to the PR.
- [x] I wrote a commit message in **English**.
- [x] I wrote a commit message according to [**the Conventional Commits
specification**](https://www.conventionalcommits.org/en/v1.0.0/).
- [x] [I added the appropriate
**changeset**](https://github.com/channel-io/bezier-react/blob/main/CONTRIBUTING.md#add-a-changeset)
for the changes.
- [ ] [Component] I wrote **a unit test** about the implementation.
- [ ] [Component] I wrote **a storybook document** about the
implementation.
- [ ] [Component] I tested the implementation in **various browsers**.
  - Windows: Chrome, Edge, (Optional) Firefox
  - macOS: Chrome, Edge, Safari, (Optional) Firefox
- [ ] [*New* Component] I added my username to the correct directory in
the `CODEOWNERS` file.

## Related Issue

- #1568

## Summary
<!-- Please add a summary of the modification. -->

- `ProgressBar` 컴포넌트의 SizeProps인 `ProgressBarSize`, VariantProps인
`ProgressBarVariant`에 enum 대신 string literal type이 들어갈 수 있도록 변경하고, enum을
deprecated 처리합니다.

## Details
<!-- Please add a detailed description of the modification. (such as
AS-IS/TO-DO)-->

## Breaking change or not (Yes/No)
<!-- If Yes, please describe the impact and migration path for users -->

- No
- 향후 `ProgressBarSize`, `ProgressBarVariant` enum 사용처에서 이를 string
literal로 변경해주어야 합니다.

## References
<!-- External documents based on workarounds or reviewers should refer
to -->
@sungik-choi sungik-choi mentioned this issue Dec 15, 2023
yangwooseong added a commit that referenced this issue Feb 8, 2024
… Banner, etc (#1984)

<!--
  How to write a good PR title:
- Follow [the Conventional Commits
specification](https://www.conventionalcommits.org/en/v1.0.0/).
  - Give as much context as necessary and as little as possible
  - Prefix it with [WIP] while it’s a work in progress
-->

## Self Checklist

- [x] I wrote a PR title in **English** and added an appropriate
**label** to the PR.
- [x] I wrote the commit message in **English** and to follow [**the
Conventional Commits
specification**](https://www.conventionalcommits.org/en/v1.0.0/).
- [x] I [added the
**changeset**](https://github.com/changesets/changesets/blob/main/docs/adding-a-changeset.md)
about the changes that needed to be released. (or didn't have to)
- [x] I wrote or updated **documentation** related to the changes. (or
didn't have to)
- [x] I wrote or updated **tests** related to the changes. (or didn't
have to)
- [x] I tested the changes in various browsers. (or didn't have to)
  - Windows: Chrome, Edge, (Optional) Firefox
  - macOS: Chrome, Edge, Safari, (Optional) Firefox

## Related Issue
<!-- Please link to issue if one exists -->

<!-- Fixes #0000 -->

- #1815
- #1568

## Summary
<!-- Please brief explanation of the changes made -->

- 컴포넌트의 enum 타입을 string literal 타입으로 변경하고 codemod 에 변경되는 속성을 추가합니다. 
- 한 pr 에 하기에는 변경사항이 많아서 2~3개로 분리해서 작업하려고 합니다.

## Details
<!-- Please elaborate description of the changes -->

- `v2-enum-member-to-string-literal` codemod 를 사용해서 하나씩 변환했습니다. 
- 스토리북에서 enum 으로부터 control option 을 얻어오는 부분은 string literal 타입이 되면 스토리북이
알아서 radio 버튼으로 바꿔주기 때문에 제거했습니다. 개발 모드에서는 input type으로 되어 직접 입력해야하지만, 크게
불편하지는 않다고 생각됩니다.
- StatusType 만 'OnlineCrescent' 처럼 PascalCase 로 되어있고 다른 부분은 kebab-case
라서 변환하면서 모두 kebab-case 로 통일해야 하나 고민입니다. 일관성 측면에서 통일하는게 좋아보입니다.



### Breaking change? (Yes/No)
<!-- If Yes, please describe the impact and migration path for users -->

- Yes

## References
<!-- Please list any other resources or points the reviewer should be
aware of -->

- None
@github-project-automation github-project-automation bot moved this from 📌 Backlog to ✅ Done in Bezier React Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issues or PR related to making existing features better
Projects
No open projects
Archived in project
3 participants