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: change the identifier of the bezier icon to a string literal instead of a symbol #1411

Merged
merged 2 commits into from
Jun 9, 2023

Conversation

sungik-choi
Copy link
Contributor

@sungik-choi sungik-choi commented Jun 9, 2023

Self Checklist

  • I wrote a PR title in English.
  • I added an appropriate label to the PR.
  • I wrote a commit message in English.
  • I wrote a commit message according to the Conventional Commits specification.
  • I added the appropriate 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

None

Summary

아이콘 컴포넌트의 식별자를 심볼에서 문자열 리터럴로 변경합니다.

Details

식별자가 심볼일 경우 아래와 같은 문제가 발생할 수 있습니다. (했습니다...)

  • A 패키지의 의존성 bezier-icons / A 패키지 내부의 B 패키지의 의존성 bezier-icons
  • A 패키지에서 import한 IconSource를, B 패키지 내부에서 import한 isBezierIcon 을 통해 아이콘의 고유성을 체크하는 경우
  • A 패키지와 B 패키지의 bezier-icons 모듈이 각기 다르기 때문에, Symbol이 2번 선언되고 아이콘 고유성 체크가 실패하게 됩니다.

bezier-react에 peer dependency로 bezier-icons를 추가하는 방식으로 문제를 해결할 수도 있습니다. 하지만 bezier-icons는 bezier-react와 꼭 쌍으로 사용해야하는 패키지가 아니고, bezier-react를 사용하는 사용처에서도 아이콘을 사용하지 않음에도 bezier-icons를 설치해야하는 불편함이 발생하게 됩니다. 또한 굳이 Symbol을 사용할 정도로 엄격하게 고유성 체크를 할 필요는 없다고 판단하여 이 PR의 해결 방식을 적용했습니다.

Breaking change or not (Yes/No)

No. (bezier-react에선 Breaking Change입니다)

References

@sungik-choi sungik-choi added the fix PR related to bug fix label Jun 9, 2023
@sungik-choi sungik-choi self-assigned this Jun 9, 2023
@changeset-bot
Copy link

changeset-bot bot commented Jun 9, 2023

🦋 Changeset detected

Latest commit: b50f614

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@channel.io/bezier-icons Minor
bezier-figma-plugin Patch
@channel.io/bezier-react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@sungik-choi sungik-choi merged commit 0e6af01 into channel-io:main Jun 9, 2023
7 checks passed
@sungik-choi sungik-choi deleted the fix/is-bezier-icons branch June 9, 2023 03:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix PR related to bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant