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.

Sign up for GitHub

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

Jump to bottom

# 699 Migrate to Monorepo #700

Merged
merged 7 commits into from
Jan 17, 2024
Merged

# 699 Migrate to Monorepo #700

merged 7 commits into from
Jan 17, 2024

Conversation

SnowSuno
Copy link
Member

@SnowSuno SnowSuno commented Jan 1, 2024 •
edited
Loading

Summary

It closes #699

pnpm workspace 기반 Monorepo 구조를 사용하기 위해 기존의 web 소스코드를 @taxi/web 하위 모듈로 이동합니다.

Further Work

@netlify Netlify
Copy link

netlify bot commented Jan 1, 2024 •
edited
Loading

✅ Deploy Preview for taxi-dev-preview ready!

Name Link
🔨 Latest commit
🔍 Latest deploy log https://app.netlify.com/sites/taxi-dev-preview/deploys/65a766b1d410ec6e7a67f055
😎 Deploy Preview https://deploy-preview-700--taxi-dev-preview.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@SnowSuno SnowSuno self-assigned this Jan 1, 2024
@SnowSuno SnowSuno requested a review from withSang January 1, 2024 11:29
@SnowSuno SnowSuno added ⚒ enhancement New feature or request help wanted Further information is requested 🖥 working 아직 작업 중인 상태 🔨refactoring 기존의 코드를 리팩토링합니다 labels Jan 1, 2024
@SnowSuno
Copy link
Member Author

SnowSuno commented Jan 1, 2024

@withSang 시간 날때 CI/CD나 netlify preview, linter 등 영향 받는 부분 트래킹하는거 한번 도와주실 수 있나요?

@14KGun
Merge pull request #701 from sparcs-kaist/dev
795bb49 
Main branch update from Dev branch
withSang
withSang reviewed Jan 3, 2024
View reviewed changes
Copy link
Member

@withSang withSang left a comment

Choose a reason for hiding this comment

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

우왕 tsconfig 손보시느라 고생하셨습니다!!

TODO: (라고 @SnowSuno 님이 말씀하셨어요)

  1. CI (cypress) 커맨드 실행 경로 수정
  2. Netlify 빌드 커맨드 수정
  3. Storybook 연동

package.json Outdated
Comment on lines 22 to 23
"cypress": "^10.3.1",
"cypress-react-selector": "^3.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

@SnowSuno : 웹 패키지로 넣어야 함
이라고 해요

@SnowSuno SnowSuno marked this pull request as ready for review January 9, 2024 02:39
@cokia cokia self-requested a review January 9, 2024 14:26
@SnowSuno
Copy link
Member Author

@withSang @cokia CI/CD 쪽 한번 봐주시면 감사하겠습니다!
netlify 빌드 커맨드도 제가 임의로 바꿔도 되는지 모르겠어서 일단 놔두었습니다

14KGun
14KGun reviewed Jan 10, 2024
View reviewed changes
.gitignore
@@ -1,7 +1,7 @@
# See https://help.github.com/articles/ignoring-files/ for more about ignoring files.

# dependencies
/node_modules
**/node_modules
Copy link
Member

Choose a reason for hiding this comment

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

이 수정이랑 마찬가지로

build 디렉토리도 .gitignore 수정해야 할 것 같아요

# production
**/build

Copy link
Member Author

Choose a reason for hiding this comment

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

반영해두었습니다

@14KGun
Copy link
Member

14KGun commented Jan 10, 2024

@SnowSuno

Dockerfile 빌드할 때, pnpm build 를 하면 package/web빌드와 package/app 빌드가 모두 될 것 같은데 Dockerfile을 package/web 과 package/app 따로 만드는 것은 어떤가욤??

@SnowSuno
Copy link
Member Author

@SnowSuno

Dockerfile 빌드할 때, pnpm build 를 하면 package/web빌드와 package/app 빌드가 모두 될 것 같은데 Dockerfile을 package/web 과 package/app 따로 만드는 것은 어떤가욤??

앱은 앱 특성상 사실 도커파일이 필요 없을 것 같고, 웹용 도커 파일에서는 pnpm -F @taxi/web build 명령어(pnpm web build로 alias 설정되어 있습니다)로 빌드하도록 수정해야 합니다.

도커파일은 일부러 따로 수정하지 않고 놔두었던 건데, 그냥 이 부분까지 이 PR에서 수정하는게 나을 듯 하네요!
도커파일까지 고쳐서 다시 리뷰 요청드릴게요!

@SnowSuno SnowSuno marked this pull request as draft January 10, 2024 09:17
SnowSuno
SnowSuno commented Jan 10, 2024
View reviewed changes
Dockerfile
Copy link
Member Author

Choose a reason for hiding this comment

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

biseo에서 현재 하고 있는 것처럼 multi-stage build를 하면 좋긴 한데,
react-inject-env 문제도 있고 eks 도입하면서 곧 그냥 s3 static serving 방식으로 변경될 것 같아서 해당 부분은 작업 안해뒀습니다.

@SnowSuno SnowSuno marked this pull request as ready for review January 10, 2024 18:26
@SnowSuno SnowSuno requested a review from 14KGun January 10, 2024 18:26
@SnowSuno
Copy link
Member Author

Dockerfile까지 업데이트해서 다시 PR 열었습니다. @14KGun

SnowSuno
SnowSuno commented Jan 10, 2024
View reviewed changes
Dockerfile
Comment on lines +19 to +20
COPY pnpm-lock.yaml .
RUN pnpm fetch
Copy link
Member Author

Choose a reason for hiding this comment

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

pnpm fetch 이용해서 디펜던시 변경 없을 때에는 해당 레이어 캐싱되도록 최적화해두었습니다
(사실 어차피 github actions 써서 빌드하면 크게 의미는 없을것 같긴 합니다)

SnowSuno
SnowSuno commented Jan 10, 2024
View reviewed changes
Dockerfile
Comment on lines +26 to +27
RUN pnpm --filter @taxi/web... install --offline; \
pnpm --filter @taxi/web... build
Copy link
Member Author

@SnowSuno SnowSuno Jan 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

아직은 @taxi/web이 의존하고 있는 패키지가 없지만 추후 생기는 것을 대비하기 위해 --filter @taxi/web 대신 --filter @taxi/web... 옵션을 주어서 의존성까지 함께 빌드될 수 있도록 해 두었습니다.

나중에 @taxi/core 패키지 만들었을 때 다시 한번 테스트는 해보아야 할 듯 합니다.

14KGun
14KGun approved these changes Jan 12, 2024
View reviewed changes
Copy link
Member

@14KGun 14KGun left a comment

Choose a reason for hiding this comment

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

수고하셨습니다 ~ 👍

@ybmin
Copy link
Contributor

ybmin commented Jan 16, 2024

LGTM
수고하셨습니당

@SnowSuno
Copy link
Member Author

netlify build 설정도 업데이트해두었습니다

@SnowSuno SnowSuno merged commit b9d456c into dev Jan 17, 2024
0 of 4 checks passed
@SnowSuno SnowSuno deleted the #699-migrate-to-monorepo branch January 17, 2024 05:49
@SnowSuno
Copy link
Member Author

#679 의 todo도 일부 해결해버린(?) 것 같네요

This was referenced Jan 17, 2024
Open
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@14KGun 14KGun 14KGun approved these changes

@withSang withSang Awaiting requested review from withSang

@happycastle114 happycastle114 Awaiting requested review from happycastle114

@ybmin ybmin Awaiting requested review from ybmin

@predict-woo predict-woo Awaiting requested review from predict-woo

@cokia cokia Awaiting requested review from cokia

Assignees

@SnowSuno SnowSuno

Labels
⚒ enhancement New feature or request help wanted Further information is requested 🔨refactoring 기존의 코드를 리팩토링합니다 🖥 working 아직 작업 중인 상태
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Migrate to monorepo
4 participants
@SnowSuno @14KGun @ybmin @withSang