Skip to content

refactor: remove namespace#243

Merged
bohongu merged 29 commits intomainfrom
refactor/remove_namespace
Feb 19, 2025
Merged

refactor: remove namespace#243
bohongu merged 29 commits intomainfrom
refactor/remove_namespace

Conversation

@TaehuiKim
Copy link
Copy Markdown
Contributor

@TaehuiKim TaehuiKim commented Feb 17, 2025 •
edited by bohongu
Loading

PR 의 종류는 어떤 것인가요?

  • 리팩토링

코드 변경을 이해하기 위한 배경지식이 필요하다면 설명 해주세요.

namespace 사용으로 인해 Tree-shaking이 되지 않는 이슈 해소를 위한 제안
전체 작업은 되지 않았고, 일부 작업만 해본 상황입니다.
만약 이것이 적용된다면 메이저 버전을 올려 적용할 예정입니다.

주요 변경점

  1. deprecated 적용 method 제거
  • DateUtil
LocalDateTimeFormatOpts (interface)
  • http-client
HttpsRes (type)
  • StringUtil
midMask
normalizeKoreaPhoneNumber
isValidPhoneNumber
escapeCsv
  1. Nullable 타입 수정, Nullish 타입 적용 https://github.com/day1coloso/coloso/pull/7036#issuecomment-2661939732
  2. 네임스페이스가 제거될 경우 다른 모듈과의 이름 충돌 가능성을 내포하고 있는 함수명 수정.
  • DateUtil
    • parse => parseDate
    • format => formatDate
    • startOf => startOfDate
    • diff => diffDate
    • min => minDate
  • StringUtil
    • join => joinStrings
    • split => splitString
  • BooleanUtil
    • valueOf => valueOfBoolean
  • MapUtil
    • get => getMapValue
  • NumberUtil
    • valueOf => valueOfNumber
  • ObjectUtil
    • isEqual => isEqualObject

@TaehuiKim TaehuiKim self-assigned this Feb 17, 2025
mitrvlr
mitrvlr approved these changes Feb 17, 2025
View reviewed changes
Copy link
Copy Markdown

@mitrvlr mitrvlr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

저는 긍정적이에요.

namespace 형태의 장점 (캡슐화, 은닉.. 등등)이 있지만 모듈 자체가 비대해진다는 점이 아쉽고, 유틸리티 성격의 라이브러리는 엔드유저에게 필요한 메서드만 제공하고 경량화되는 방향을 더 선호하기도 합니다

cc @soomtong @9w

soomtong
soomtong approved these changes Feb 17, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@soomtong soomtong left a comment

Choose a reason for hiding this comment

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

동의합니다. keep going.

kevinGwon
kevinGwon approved these changes Feb 17, 2025
View reviewed changes
Copy link
Copy Markdown

@kevinGwon kevinGwon left a comment

Choose a reason for hiding this comment

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

LGTM!

이견 없습니다!

elegantcoder
elegantcoder approved these changes Feb 17, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@elegantcoder elegantcoder left a comment

Choose a reason for hiding this comment

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

좋아보입니다.

9w
9w approved these changes Feb 17, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@9w 9w left a comment

Choose a reason for hiding this comment

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

LGTM

@knoa0405
Copy link
Copy Markdown

knoa0405 commented Feb 17, 2025

LGTM

bohongu
bohongu approved these changes Feb 17, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@bohongu bohongu left a comment

Choose a reason for hiding this comment

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

LGTM

sunivers
sunivers approved these changes Feb 17, 2025
View reviewed changes
Copy link
Copy Markdown

@sunivers sunivers left a comment

Choose a reason for hiding this comment

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

LGTM!! 👍

@TaehuiKim TaehuiKim force-pushed the refactor/remove_namespace branch from 4fadfd6 to 78cca6a Compare February 17, 2025 04:53
@TaehuiKim
Copy link
Copy Markdown
Contributor Author

TaehuiKim commented Feb 17, 2025

@bohongu 이 뒤로는 재홍님께서 작업해주실 예정입니다, 감사합니다
cc @mitrvlr @kevinGwon

TaehuiKim
TaehuiKim commented Feb 17, 2025
View reviewed changes
src/index.ts Outdated
export type { ByteUnitType, UnitConverter } from './unit-util';
export { MoneyUtil } from './money-util';

export { UTF8_BOM_STR } from './string-util';
Copy link
Copy Markdown
Contributor Author

@TaehuiKim TaehuiKim Feb 17, 2025

Choose a reason for hiding this comment

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

위에서 와일드카드로 export 선언 된 것들이 있는데, 혹시 한 번 더 선언해준 이유가 별도로 있을까요...?

Copy link
Copy Markdown
Contributor

@bohongu bohongu Feb 17, 2025

Choose a reason for hiding this comment

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

제거하였습니다

@bohongu bohongu force-pushed the refactor/remove_namespace branch from 7378197 to c176513 Compare February 17, 2025 06:59
TaehuiKim
TaehuiKim commented Feb 17, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor Author

@TaehuiKim TaehuiKim left a comment

Choose a reason for hiding this comment

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

버전만 메이저로 지정해주시면 될 것 같습니다...! 고생 많으셨습니다, 재홍님!

src/type-util/type-util.type.ts Outdated
};
export type Nullable<T> = T | null | undefined;

// https://github.com/day1coloso/coloso/pull/7036#issuecomment-2661939732
Copy link
Copy Markdown
Contributor Author

@TaehuiKim TaehuiKim Feb 17, 2025

Choose a reason for hiding this comment

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

p3: pebbles는 public repo이기 때문에, 다른 사람들이 볼 수 없는 맥락의 주석보다는 설명을 조금 추가해주는 것이 더 좋을 것 같습니다.

plainOldCode
plainOldCode approved these changes Feb 17, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@plainOldCode plainOldCode left a comment

Choose a reason for hiding this comment

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

LGTM
4.0.0 으로 올리는 서비스 및 프로젝트들은 해당 모듈에 대한 테스트 검증을 확실하게 해야할 것으로 보입니다.

sujin-park
sujin-park approved these changes Feb 17, 2025
View reviewed changes
Copy link
Copy Markdown

@sujin-park sujin-park left a comment

Choose a reason for hiding this comment

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

LGTM!

@hibiyasleep hibiyasleep mentioned this pull request Feb 17, 2025
Merged
@hibiyasleep
Copy link
Copy Markdown
Contributor

hibiyasleep commented Feb 17, 2025

See also: #244

@bohongu bohongu force-pushed the refactor/remove_namespace branch from 3ba58ed to f0780be Compare February 18, 2025 07:29
@bohongu bohongu force-pushed the refactor/remove_namespace branch from f0780be to 362dd49 Compare February 18, 2025 07:41
@hibiyasleep hibiyasleep force-pushed the refactor/remove_namespace branch from ec9520e to 925ea4a Compare February 19, 2025 04:33
@bohongu bohongu merged commit 44c2ef8 into main Feb 19, 2025
1 check passed
@bohongu bohongu deleted the refactor/remove_namespace branch February 19, 2025 08:41
@bohongu bohongu mentioned this pull request Feb 20, 2025
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bohongu bohongu bohongu approved these changes

@soomtong soomtong soomtong approved these changes

@elegantcoder elegantcoder elegantcoder approved these changes

@hibiyasleep hibiyasleep hibiyasleep approved these changes

@Q00 Q00 Awaiting requested review from Q00

@alstjs1207 alstjs1207 Awaiting requested review from alstjs1207

@doyoonear doyoonear Awaiting requested review from doyoonear

@jaewon-noh jaewon-noh Awaiting requested review from jaewon-noh

+6 more reviewers

@plainOldCode plainOldCode plainOldCode approved these changes

@9w 9w 9w approved these changes

@mitrvlr mitrvlr mitrvlr approved these changes

@sunivers sunivers sunivers approved these changes

@sujin-park sujin-park sujin-park approved these changes

@kevinGwon kevinGwon kevinGwon approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@TaehuiKim TaehuiKim

Labels

maintenance

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.