Conversation
|
🚀 Playground 프리뷰 배포 확인하기 🚀 |
3b2ad9e to
97b90bf
Compare
There was a problem hiding this comment.
lint staged는 기본적으로 매칭된 파일들을 커맨드 뒤에 이어붙히는데, typecheck 시 뒤에 파일들이 잇다라 붙게되면 turbo가 이를 인식하지 못해 에러가 발생합니다.
.js로 함수형으로 작성하였을 때 커맨드 온리로 실행할 수 있어 변경합니다.
There was a problem hiding this comment.
아하! 이 부분이 lint staged가 파일을 자동으로 붙이면서 앞에 --filter 옵션과 충돌하는 이유때문에 바꾸신거군요! 처음 알게 된 사실이네요! 굿굿! 👍 👍
97b90bf to
76a11a8
Compare
번들링에 대한 내용이 dist로 build된 결과물을 import해서 사용하고 있었던 부분이 맞을까요? 이를 단순 workspace로 참조하도록 한다고 이해했는데, dist로 빌드한 결과물을 제품에서 참조해도 번들에 포함되는 것이 아닌가라는,,? 생각이 들어서 dist와 workspacee에 대해 장단점은 뭔지, trade-off가 각각 있는지 궁금합니다! 그리고 추가로 모노레포로 디자인시스템 등 패키지에 추가할때 어떤 방법을 많이 쓰는지도 궁금해요! |
constantly-dev
left a comment
There was a problem hiding this comment.
고생하셨습니다!! 제 생각에도 UI와 링크등의 상수는 분리되는 것이 책임 측면에서 좋다고 생각이 들어서 적절한 분리였다고 생각합니다.
간단한 코멘트들 몇개 달았으니 확인부탁드려요! 👍
| }, | ||
| ], | ||
| '@typescript-eslint/no-empty-function': 'off', | ||
| '@typescript-eslint/no-empty-interface': 'off', |
There was a problem hiding this comment.
요거 rule off한 특별한 이유가 있을까요?
There was a problem hiding this comment.
no empty function은 사실 지금껏 이 옵션을 on한 적이 없기도 하고 너무 좁혀진 옵션이라고 생각해서 껐어요.
no empty interface는 사실 켯껏 둘다 상관없다고 생각하긴 했는데, 기존에 interface {}로 사용하던 곳들이 많아서서 off 해주었습니다.
on 에 대한 의견 있으시면 말씀해주세용
| @@ -0,0 +1 @@ | |||
| export { playgroundLink, crewLink, MAKERS_TEAM_URL } from './links'; | |||
There was a problem hiding this comment.
playground-common에 UI, 상수 관련 코드를 각각 packages/ui, packages/constant로 분리하셨다는 말로 이해했는데 맞을까요??!
물론 두 팀이 서로의 팀 link를 공유하는 상황도 있긴 하겠지만 그렇게 많지는 않을 것 같은데, 이를 packages에 올리신 기준이 따로 있으신지 궁금합니다!
There was a problem hiding this comment.
저도 이 부분이 궁금합니다 !! packages로 따로 올리신 기준이 궁금해요 !!
There was a problem hiding this comment.
사실 crew랑 playground가 서로에 대한 리디렉션 떄문에 상수를 공유할 필요가 있다고 판단해서 빼주었습니다. 물론 저는 말씀해주신 것처럼 어느 정도 빈번하게 사용되는지는 잘 모르지만, 앞으로 제품이 확장되면서 많이 참조될 가능성이 있다고해서 이참에 분리해주었어요.
There was a problem hiding this comment.
아하! 이 부분이 lint staged가 파일을 자동으로 붙이면서 앞에 --filter 옵션과 충돌하는 이유때문에 바꾸신거군요! 처음 알게 된 사실이네요! 굿굿! 👍 👍
| "lint": { | ||
| "cache": false, | ||
| "outputs": [] | ||
| }, | ||
| "stylelint": { | ||
| "cache": false, | ||
| "outputs": [] | ||
| }, | ||
| "format": { | ||
| "cache": false, | ||
| "outputs": [] | ||
| }, | ||
| "typecheck": { | ||
| "cache": false, | ||
| "outputs": [] | ||
| }, |
There was a problem hiding this comment.
사실상 outputs가 없는 script들이라 cache가 애초에 있어도 뭘 캐싱하지 싶었는데, 공식문서를 참고하니 outputs와 log를 캐싱한다고 되어있네요! 그래서 log를 캐싱하기는 하지만 주용님의 의도가 format이나 린팅 같은 경우는 무조건 필수로 돌게 하는 것이 의도이신 것 같아요. (맞을까요?!) 맞다면 저도 해당 의견에 동의합니다~!
https://turborepo.dev/docs/crafting-your-repository/caching?utm_source=chatgpt.com#what-gets-cached
sonnnnhe
left a comment
There was a problem hiding this comment.
고생하셨습니다 ! 👍 👍
PR과는 직접적인 연관이 있는 질문은 아니지만
제가 .md 파일로 에이전트용 스킬을 설정하는 걸 makers에서 처음 접해서 그런데
코드 리뷰할 때 어떤 부분을 중심으로 어떻게 봐야하는지가 궁금합니다...
SKILL.md 내용을 바탕으로 references/ 하위 파일들의 흐름을 보면 되는 것일까요?
(references/ 하위 파일들을 리뷰할 때 신경써야 하는 게 어떤 게 있을지도 궁금해요 ..)
jogpfls
left a comment
There was a problem hiding this comment.
고생 많으셨습니다 !! 👍
궁금한 부분 몇 개만 코멘트로 남겼는데 확인해주시면 감사하겠습니다 !!
| @@ -0,0 +1 @@ | |||
| export { playgroundLink, crewLink, MAKERS_TEAM_URL } from './links'; | |||
There was a problem hiding this comment.
저도 이 부분이 궁금합니다 !! packages로 따로 올리신 기준이 궁금해요 !!
| @@ -0,0 +1,4 @@ | |||
| import { ReactElement, ReactNode } from 'react'; | |||
There was a problem hiding this comment.
packages 내부에 있는 타입들은 import type이 안된 것 같은데 한 번 확인 부탁드려요!
There was a problem hiding this comment.
오 그렇네요. 음 패키지에도 lint를 넣어야하겠네요. 감사합니다 !
| @@ -1,51 +1,8 @@ | |||
| export { crewLink, MAKERS_TEAM_URL, playgroundLink } from '@sopt/constant'; | |||
There was a problem hiding this comment.
constants/links.ts를 통해 한 번 re-export하는 구조인 것 같아요 !
현재 일부 파일은 @sopt/constant를 직접 import하고, 일부는 @/constants/links를 사용하고 있어서 import 경로가 일관되지 않은 것 같은데 의도하신 구조일까요??
There was a problem hiding this comment.
그렇네요..! 이걸 발견하시다니 .. 감사합니다 다시 수정하겠습니답
ui 패키지가 번들링을 하지 않고, 이를 사용하는 제품처에서 번들링한다라는 말이였습니다. 결국에는 ui 패키지가 번들링할 필요성이 없다고 판단했기 때문에 기존에 tsup을 활용한 dist 참조 방식을 없앴어요. 다양한 모듈 스펙들을 지원해야한다거나, cjs 온리 라이브러리를 사용하고 있다거나, 구형 브라우저에 대응해야하는 등의 상황도 아닐뿐더러, private한 패키지가 타겟할 제품들과 es 모듈임이 명확하기 때문에 오히려 복잡성만 증가한다고 판단했습니다. 오히려 next를 사용하는 crew와 playground의 번들러(webpack ? )를 존중하는 것이 복잡성도 낮추고 에러 발생 가능성도 줄일 수 있다 판단했습니다. trade-off보다는 상황에 따라 장/단점을 보고 결정하는 것 같아용 |
음 스킬 리뷰라는게 좀 주관적일 수도 있을 것 같은데요. 사실 정답이 있다기보다는 가이드 정도 제공하고 있는 상황이라서, 지금 당장 내 피드백으로 에이전트의 사고나 산출물 완성도가 높아진다면 리뷰가 가능하겠지만 결국 사용자도 에이전트나 스킬을 사용해보며 경험으로부터 고도화하는 거라고 생각해서 이런 것들이 잘 리뷰가 될지는 모르겠네요. 물론 명확히 이렇게 했을 때 에이전트 성능이 높아진다고 판단이 되면 당연히 리뷰해주시면 좋습니다. 레퍼런스의 경우는 저게 버셀에서 제공하고 있는 스킬인데, 버셀 측에서 에이전트가 이 스킬을 로드하여 사용할 때 필요할 수도 있겠다 싶은 레퍼런스를 모아둔 거에요. 이 또한 위 맥락처럼 주도적으로 리뷰해주셔도 좋지만, 저명한 오픈소스들이 내놓은 스킬은 일단 믿고 쓰는 편이긴 합니다..! |
📋 작업 내용
📌 PR Point