-
Notifications
You must be signed in to change notification settings - Fork 46
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
Enhance build time for bezier-react #1424
Enhance build time for bezier-react #1424
Conversation
…te type declaration files
…ion process (WIP)
🦋 Changeset detectedLatest commit: 5dc1e54 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
/** | ||
* FIXME: https://github.com/rollup/plugins/issues/872 | ||
* @rollup/plugin-commonjs 의 버그로 인해 default export 가 namespace 그 자체로 계산됨. | ||
* commonjs 상황을 위해 '.default' 를 추가함. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rollup/plugins#1165 에서 수정됨
Chromatic Report🚀 Congratulations! Your build was successful! |
}, | ||
}, | ||
presets: [ | ||
['@babel/preset-env', { useBuiltIns: 'entry', corejs: '3.31.0', bugfixes: true }], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@babel/plugin-proposal-private-property-in-object
,@babel/plugin-proposal-class-properties
플러그인은preset-env
ES2022에 내장되었으므로 제거- 폴리필을 위한 corejs 활성화
- 번들 사이즈 감소를 위한 bugfixes 옵션 활성화 (More info: https://github.com/babel/preset-modules)
…ts about order matters
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1424 +/- ##
=======================================
Coverage 84.57% 84.57%
=======================================
Files 308 308
Lines 3909 3909
Branches 792 791 -1
=======================================
Hits 3306 3306
Misses 532 532
Partials 71 71
☔ View full report in Codecov by Sentry. |
"module": "build/src/index.js", | ||
"types": "build/src/index.d.ts", | ||
"main": "dist/cjs/index.js", | ||
"module": "dist/esm/index.mjs", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type="module"
이 아니라면, 즉, CJS 패키지라면 ESM 모듈엔 mjs
로 분명하게 확장자를 명시해주어야합니다. bezier-icons 패키지에도 마찬가지로 적용되어 있습니다.
"src" | ||
], | ||
"scripts": { | ||
"build": "cross-env BABEL_ENV=production rollup -c", | ||
"build": "run-p 'build:*'", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rollup을 통한 자바스크립트 번들링, ttsc(ttypescript)를 통한 타입 생성을 병렬로 수행합니다.
"peerDependencies": { | ||
"@channel.io/bezier-icons": ">=0.2.0", | ||
"react": "^16.8.0 || ^17.0.0 || ^18.0.0", | ||
"react-dom": "^16.8.0 || ^17.0.0 || ^18.0.0", | ||
"styled-components": ">=5" | ||
}, | ||
"peerDependenciesMeta": { | ||
"@channel.io/bezier-icons": { | ||
"optional": true | ||
} | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why move bezier-icons from deps to peerDeps
bezier-react에 peer dependency로 bezier-icons를 추가하는 방식으로 문제를 해결할 수도 있습니다. 하지만 bezier-icons는 bezier-react와 꼭 쌍으로 사용해야하는 패키지가 아니고, bezier-react를 사용하는 사용처에서도 아이콘을 사용하지 않음에도 bezier-icons를 설치해야하는 불편함이 발생하게 됩니다.
peerDependenciesMeta
옵션 설정을 통해, bezier-icons가 설치된 패키지에서만 fix: change the identifier of the bezier icon to a string literal instead of a symbol #1411 의 문제가 발생하지 않는 버전을 강제할 수 있습니다. 따라서 위에 언급한 불편함이 어느정도 해소될 거라고 판단했습니다.- 불필요한 트랜스파일링 과정을 거치지 않아도 됩니다. (babel에 exclude 옵션이 있으나, yarn workspace 내부 패키지에는 symlink로 연결되어 잘 적용되지 않는 문제가 있었습니다.)
- 내부적으로 ChevronIcon을 사용하는 Select 같은 컴포넌트의 케이스에서 bezier-icons 의존성 추가없이 사용할 경우 문제가 될 수 있습니다. 이 경우 개발 서버 및 빌드 과정에서 module resolve 에러가 발생할 것이므로 프로덕션에서 문제가 발생할 가능성은 낮다고 판단했습니다.
alias({ | ||
entries: [{ | ||
find: '~', | ||
replacement: rootDir, | ||
}], | ||
}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- AS-IS: rollup typescript plugin + ttypescript & typescript-transform-paths 를 사용하여 자바스크립트 + 타입 선언의 절대 경로를 resolve
- TO-BE: rollup alias plugin 을 통해 자바스크립트의 절대 경로를 resolve 하고, ttypescript & typescript-transform-paths 를 사용하여 타입 선언의 절대 경로를 resolve
"exclude": [ | ||
"node_modules", | ||
"build", | ||
"coverage", | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.eslintignore
파일에 명시되어있으므로 불필요합니다.
e98fc1a
to
01b53c2
Compare
"sideEffects": false, | ||
"files": [ | ||
"build", | ||
"dist", | ||
"!dist/*.tsbuildinfo", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dist 내부에 포함되어있는 ts build log는 파일에 포함하지 않습니다.
"*.ts", | ||
"*.js", | ||
".*.js", | ||
"src/**/*", | ||
"scripts/**/*", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
불필요한 glob (icons 분리 전에 필요했음)
<!-- 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 #1005 ## Summary <!-- Please brief explanation of the changes made --> - 빌드 잡을 CI에 추가합니다. - 빌드 잡에 캐시를 추가합니다. ## Details <!-- Please elaborate description of the changes --> 기존에 `--filter=bezier-icons` 를 통해 아이콘 패키지만 빌드하던 것에서 필터를 제거하고, 모든 패키지를 빌드하도록 변경합니다. 의존성 업데이트 등 변경이 있었을 경우 라이브러리가 정상적으로 빌드되는 지 검증하기 위해서입니다. #1424 에서 빌드 시간이 많이 단축되었지만, 필터가 제거되었으므로 총 빌드 시간도 유의미하게 증가하게 될 거라 생각했습니다. 이를 방지하고자 빌드 잡에 캐시를 추가했습니다. ### 빌드 잡에 캐시 추가 - 기존엔 jest의 캐시가 없었습니다. 캐시 관련 설정을 추가합니다. - 마이너: 캐시 설정을 추가하며 jest 설정을 레포지토리 전반적으로 통일합니다. jest 관련 패키지를 루트로 옮기고, `@swc/jest` 를 모든 패키지에 적용하여 테스트 수행시간을 줄이고자 했습니다. 기본값과 동일하거나 불필요한 jest 설정 옵션은 제거했습니다. - `test:ci` 스크립트를 제거하고 기존 `test: jest --onlyChanged` 스크립트를 `test:ci` 와 동일한 스크립트로 변경합니다. 기존에 only changed 플래그가 추가되었던 이유가 pre-commit 훅을 빠르게 수행하기 위해서였는데, pre-commit 훅이 제거되었기때문에 불필요해졌다고 판단했습니다. ### Breaking change? (Yes/No) <!-- If Yes, please describe the impact and migration path for users --> No
Self Checklist
CODEOWNERS
file.Related Issue
Summary
bezier-react 패키지의 빌드 과정을 개선합니다.
Details
변경 전/후 빌드 시간 측정
bezier-react
bezier-icons
테스트 환경
변경 전/후 bezier-react 번들 사이즈 측정
Breaking change or not (Yes/No)
Yes. bezier-react의 peer dependency로
@channel.io/bezier-icons >= 0.2.0
이 추가되며, 더 이상 번들에 포함되지 않습니다.References
참고할만한 레퍼런스는 셀프 코드 리뷰로 남깁니다.