refactor(view/engine): DI 컨테이너 마이그레이션 (tsyringe -> inversify)#898
Conversation
- Replace tsyringe with inversify in view package - Update IDE adapters and service layer container usage
- Organize DI container logic into dedicated container folder structure - Replace string-based tokens with Symbol.for() tokens - Add webpack path aliases for container and ide directories - Create centralized service token definitions in container/tokens.ts
- Replace tsyringe with inversify for dependency injection - Add explicit container binding configuration - Update decorators from @singleton() to @Injectable()
ytaek
left a comment
ytaek
left a comment
There was a problem hiding this comment.
새로운 lib으로 변경하느라 고생하셨습니다! inversify.js를 잘 이해해서 적용시켜주신 것 같네요!!
그리고 사소한건데,
이 PR이 전체 변경량이 작긴 하지만, view와 engine을 별도로 올려주셨으면 리뷰할 때 조금 편하지 않았을까 싶습니다 😸
(monorepo 상에서 하는거라 좀 헷갈리는 부분도 있을 듯 하네요)
우선 머지하시고, 커멘트 나온 것들 중 필요한 부분들은 이슈로 재정리 한다음에 다시 작업해주시면 좋을 것 같습니다!!
(아 그리고 container를 쓴 다른 tc는 없었을까용?)
| "@octokit/types": "^13.5.0", | ||
| "reflect-metadata": "^0.1.13", | ||
| "tsyringe": "^4.7.0" | ||
| "inversify": "^7.10.0", |
|
|
||
| import PluginOctokit from "./pluginOctokit"; | ||
|
|
||
| export const container = new Container(); |
There was a problem hiding this comment.
(사소) container에 DI 이외에도 다른 container들이 있을 수 있어서
추후에 diContainer 등의 별도의 이름으로 변경하면 좋을 것 같습니다!
There was a problem hiding this comment.
동의합니다. 다음 리팩토링 때 반영하겠습니다!
| commitDetails: PullsListCommitsResponseData | ||
| }[]> => { | ||
| public getPullRequests = async (): Promise< | ||
| { |
There was a problem hiding this comment.
어라 linting이 원래 이렇게 되어 있을까요?
{ 의 시작이 첫줄로 되어 있지 않은것 같았느데요?
| export const SERVICE_TOKENS = { | ||
| OctokitOptions: Symbol.for("OctokitOptions"), | ||
| } as const; |
There was a problem hiding this comment.
(궁금) 이 심볼만 별도의 변수로 뺀 이유가 있을까요?
| export const SERVICE_TOKENS = { | ||
| IDEAdapter: Symbol.for("IDEAdapter"), | ||
| } as const; |
There was a problem hiding this comment.
여기도 별도로 뺀 이유가 있을까요?
그리고 service_token이라는 naming이 좀 더 명확하면 좋을 것 같습니다.
처음에 저는 github token같은 거라고 생각했는데,
view에 도 있는걸 보니 그건 아닌것 같네요.
There was a problem hiding this comment.
단순히 container symbol을 모아 놓는거라고 봐야할까요?
There was a problem hiding this comment.
네 container 식별자를 모아놓는 파일입니다. 추후에 늘어날 가능성을 대비해서 중복 방지나 유지보수를 위해 한 곳에서 관리할 수 있게 분리하였습니다.
네이밍은 다음 리팩토링 때 반영하도록 하겠습니다!
chae-dahee
left a comment
chae-dahee
left a comment
There was a problem hiding this comment.
string 형식의 container 식별자를 상수로 묶어서 개선한 방식이 좋네요 👍
마이그레이션도 놓치지 않고 잘 해주신 것 같습니다~ LGTM! 고생하셨습니다ㅎㅎ
3 participants
Related issue
Closes #866
Result
기존 tsyringe 기반 DI 시스템을 inversify로 교체하여 좀 더 명시적이고 타입 안전한 DI 컨테이너를 구현했습니다.
Work list
view/engine 공통
tsyringe→inversifyreflect-metadata버전 업그레이드: ^0.1.13 -> ^0.2.2 (inversify 호환성)1. view 패키지 마이그레이션
src/container/index.ts)src/container/tokens.ts)2. analysis-engine 패키지 마이그레이션
src/container.ts)src/serviceTokens.ts)src/pluginOctokit.ts데코레이터@singleton()→@injectable()전환src/container.ts에 PluginOctokit 명시적 바인딩 추가rebindSync()패턴 적용3. view 패키지 App 컴포넌트 관심사 분리
개선 효과