Skip to content

Conversation

@annie1229
Copy link
Contributor

@annie1229 annie1229 commented Dec 20, 2022

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

Fixes #1027

Summary

  • lodash-es가 사용되던 로직을 직접 구현한 util 함수로 변경하고, lodash-es를 제거합니다.
as-is (1.06MB) to-be (944.21KB)
lodash-es before removed2 lodash-es after removed

(참고) 기존에 올렸었던 as-is(붉은색 사진)은 yarn clean을 하지 않은 상태로 빌드되었었습니다.
yarn clean -> yarn install -> yarn build 동일한 과정으로 다시 측정한 파일로 다시 올렸습니다!

Details

Breaking change or not (Yes/No)

No

References

@annie1229 annie1229 added enhancement Issues or PR related to making existing features better build Issue or PR related to build system labels Dec 20, 2022
@annie1229 annie1229 self-assigned this Dec 20, 2022
@changeset-bot
Copy link

changeset-bot bot commented Dec 20, 2022

🦋 Changeset detected

Latest commit: 48f1c05

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

This PR includes changesets to release 2 packages
Name Type
@channel.io/bezier-react Patch
bezier-figma-plugin 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

@annie1229 annie1229 force-pushed the enhance/migration-lodash-es branch from 0e5682b to 4811263 Compare December 20, 2022 08:25
@codecov
Copy link

codecov bot commented Dec 20, 2022

Codecov Report

Base: 83.47% // Head: 83.60% // Increases project coverage by +0.13% 🎉

Coverage data is based on head (48f1c05) compared to base (3224af6).
Patch coverage: 88.83% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff             @@
##           next-v1    #1079      +/-   ##
===========================================
+ Coverage    83.47%   83.60%   +0.13%     
===========================================
  Files          285      289       +4     
  Lines         3570     3709     +139     
  Branches       714      769      +55     
===========================================
+ Hits          2980     3101     +121     
- Misses         517      533      +16     
- Partials        73       75       +2     
Impacted Files Coverage Δ
.../bezier-react/src/components/Toast/ToastContext.ts 0.00% <0.00%> (ø)
...ckages/bezier-react/src/foundation/GlobalStyle.tsx 33.33% <0.00%> (-9.53%) ⬇️
...rc/components/Forms/Inputs/TextField/TextField.tsx 58.82% <50.00%> (+0.34%) ⬆️
packages/bezier-react/src/utils/storyUtils.ts 87.50% <50.00%> (+1.78%) ⬆️
packages/bezier-react/src/utils/objectUtils.ts 77.35% <77.35%> (ø)
packages/bezier-react/src/utils/stringUtils.ts 66.66% <82.35%> (+6.66%) ⬆️
packages/bezier-react/src/utils/typeUtils.ts 90.90% <90.90%> (ø)
...ier-react/src/components/Avatars/Avatar/Avatar.tsx 85.29% <100.00%> (+0.44%) ⬆️
...src/components/Avatars/AvatarGroup/AvatarGroup.tsx 86.95% <100.00%> (ø)
...ages/bezier-react/src/components/Banner/Banner.tsx 92.59% <100.00%> (ø)
... and 47 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@annie1229 annie1229 force-pushed the enhance/migration-lodash-es branch from 4811263 to 5072bd5 Compare December 21, 2022 04:57
@github-actions
Copy link
Contributor

github-actions bot commented Dec 21, 2022

Chromatic Report

🚀 Congratulations! Your build was successful!

@annie1229 annie1229 force-pushed the enhance/migration-lodash-es branch 3 times, most recently from 440ddb0 to bf3adfb Compare December 22, 2022 07:32
@annie1229 annie1229 marked this pull request as ready for review December 22, 2022 08:41

filePaths.forEach(filePath => {
const basename = path.basename(filePath, path.extname(filePath))
const kebabCase = (value) => value
Copy link
Contributor

Choose a reason for hiding this comment

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

forEach callback 함수 바깥(defaultIndexTemplate 바깥)으로 분리할 수 있을 거 같습니다.

onClick = noop,
onMouseEnter = noop,
onMouseLeave = noop,
onClick = () => {},
Copy link
Contributor

Choose a reason for hiding this comment

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

noop 라는 함수가 별도로 있으면 좋겠네요! (+ 테스트 코드)

) : VariantTuple[] {
function toArray<T>(items: T | T[]): T[] {
return isArray(items) ? items : [items]
return Array.isArray(items) ? items : [items]
Copy link
Contributor

Choose a reason for hiding this comment

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

is*** 같은 함수들이 typeUtils 에 모두 구현되어있는 편이 사용하는 입장에서 예측 가능할 거 같아요.

@sungik-choi
Copy link
Contributor

sungik-choi commented Dec 26, 2022

어디까지 유틸함수화할 것인가라고 생각해보면 어렵네요. 예를 들어 Object.entries 같은 메서드를 entries 라는 별도의 유틸함수로 분리하고, 테스트를 추가하는 게 맞을지..? 다른 분들 의견도 궁금합니다.

@annie1229 annie1229 force-pushed the enhance/migration-lodash-es branch 4 times, most recently from 251b639 to 281f35c Compare January 2, 2023 09:05
@sungik-choi sungik-choi requested a review from jeff-tbd January 5, 2023 06:07
Copy link
Contributor

@sungik-choi sungik-choi left a comment

Choose a reason for hiding this comment

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

👍👍👍

Copy link
Contributor

@Dogdriip Dogdriip left a comment

Choose a reason for hiding this comment

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

👍

return !value
}

export function noop() {}
Copy link
Contributor

Choose a reason for hiding this comment

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

마이너 코멘트: noop 함수가 typeUtils에 있는 이유가 따로 있을까요? 타입을 체크하는 함수는 확실히 아닌 것 같아서, functionUtils 등 다른 파일로 빼는 게 어떨까 제안합니다.

Copy link
Contributor

@CHEWCHEWW CHEWCHEWW left a comment

Choose a reason for hiding this comment

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

👍👍👍

@annie1229 annie1229 force-pushed the enhance/migration-lodash-es branch from 345e096 to e6e86fb Compare February 24, 2023 07:21
@annie1229 annie1229 merged commit 85d04e7 into channel-io:next-v1 Feb 24, 2023
@annie1229 annie1229 deleted the enhance/migration-lodash-es branch February 24, 2023 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build Issue or PR related to build system enhancement Issues or PR related to making existing features better

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

Remove and re-implement lodash-es

6 participants