Amazon 1, Step3 - Plans 구현#44
Conversation
|
https://pdvonzoo.github.io/javascript-amazon/ |
crongro
left a comment
There was a problem hiding this comment.
하드코어 타입체크 시스템 잘 봤어요. 고생하셨네요 ㅎ
멋진 시스템이고 js를 더 잘 이해하셨을 거 같네요.
사용법은 좀 어려운 점이 있습니다.
너무 많은 클래스와 메서드간의 관계가 있어서 그럴 수 있을거 같네요.
또한 중복코드와 메서드이름에 조금더 신경쓰면 좋겠고요.
이 타입시스템을 다른 클래스와 메서드에서도 쓸 수 있게 한다고 생각하고, 좀 더 직관적인 API를 제공하면 매우 훌륭할 거 같네요.
타입체크 테스트는 이제 충분히 하신 거 같네요.
앞으로는 UI요소들의 메서드들 테스트도 해보세요. DOM에 어떤 변화를 테스트하고, 이벤트가 잘 실행됐는지 등에 대해서요.
실제로 자바스크립트 개발단계에서 타입을 이렇게 밀도있게 테스트하는 경우는많지는 않아요.(하기도 어렵고요)
| @@ -0,0 +1,90 @@ | |||
| const path = require("path"); | |||
| const MiniCssExtractPlugin = require("mini-css-extract-plugin"); | |||
| const devMode = process.env.NODE_ENV !== 'production'; | |||
There was a problem hiding this comment.
node 환경설정에 대해서 공부해둘 필요가 있습니다.
env가 의미하는 것이 무엇이고, production이 무엇인지 잘 이해하고 쓰도록 해보세요.
특히 nodejs에서 .env 파일에 대해서도 활용방법을 알아두시고요.
| }, | ||
| devtool: 'source-map', | ||
| mode : devMode ? 'development' : 'production', | ||
| module: { |
There was a problem hiding this comment.
모듈 loader는 webpack의 가장 핵심기능이죠. 각각의 의미를 잘 알아두고, 필요한 모듈을 로딩할때 잘 활용하도록 하세요.
| contentBase: "dist", | ||
| overlay: true | ||
| }, | ||
| devtool: 'source-map', |
There was a problem hiding this comment.
source-map이 어떻게 디버깅이 가능한지? 에 대해서 좀 어려운 점도 있지만,
언젠가는 들여다 보세요.
| helpers.on(els.close, 'click', _ => { | ||
| helpers.removeClass(eachCoverEls, className.clicked); | ||
| }) | ||
| return this; |
There was a problem hiding this comment.
chaining 때문에 this를 반환하나요? 괜찮은 습관이긴해요. 잘 했네요.
| this.setEvent(this.helpers, els, className, target, eachCoverEls); | ||
| } | ||
| setEvent(helpers, els, className, target, eachCoverEls){ | ||
| helpers.on(window, 'scroll', _ =>{ |
| fixPoint: els.aboutBtn.offsetTop + els.aboutBtn.offsetHeight | ||
| } | ||
| const eachCoverEls = [ els.submitCover, els.frontCover ]; | ||
| this.setEvent(this.helpers, els, className, target, eachCoverEls); |
There was a problem hiding this comment.
setEvent 는 인자의 길이가 너무 길어서 보기 어려운 면이 있군요.
좀 줄이면 좋겠네요. 어렵다면 객체로 표현하는 것도 방법이고요.
| @@ -0,0 +1,16 @@ | |||
| class PlansTypes { | |||
There was a problem hiding this comment.
전체적으로 Type 체크시스템을 만든거 같아서 매우 훌륭하게 생각하고요 🥇
실무에서 라이브러리가 아니라면 이정도 체크는 잘 하지 않고, (라이브러리에 대한 올바른 사용을 판단해서 개발자들에게 알려주기 위함)
서비스 레벨에서 복잡한 모듈관계가 보인다면, TypeScript를 사용하고 있죠.
다음프로젝트에서는 Typescript사용해보는 것을 추천합니다.
There was a problem hiding this comment.
감사합니다. 안그래도 typescript를 이용할까 고민했었는데요. 이번엔 직접 구현해보고 싶어서 시스템을 만들어 보았습니다. 다음 프로젝트에는 typescript를 적용해보도록 하겠습니다. :)
| }) | ||
| }) | ||
| beforeAll(() => { | ||
| dom = render(` |
There was a problem hiding this comment.
beforeall 단계에서, dom 설정 좋네요.
jest를 사용하면 가상돔을 제공하던데 이렇게 해도 될 듯.
| plans.controllStickyNav(el, target, currentTop, 'test') | ||
| expect(spy).toHaveBeenCalled(); | ||
| }) | ||
| it('currentTop이 target보다 값이 더 작으면 helpers 인스턴스의 remove 메서드가 실행된다.', () => { |
There was a problem hiding this comment.
어떤 하위메서드가 실행된다, 라는 것도 중요한 테스트항목은 맞고요.
우리가 더 고민 할 부분은, addClass로인해서 어떤 css속성의 변화가 실제 적용됐는지 에 있다고 생각해요.
의미적으로 더 구체적인 대상을 테스트하면 좋을 거 같네요.
There was a problem hiding this comment.
class가 추가되고 제거되는 콜백 함수가 이 메서드의 기능에 포함되었는데요.
콜백 함수는 Helpers의 기능이기 때문에 이 메서드에서는 콜백이 호출되는지만 파악하고 해당 함수(addTest, removeTest)의 테스트는 Helpers.test에서 하는 것이 맞다고 판단 했었습니다.
피드백 주신 내용을 보고 다시 고민해보니 직접적으로 Dom을 컨트롤하는 테스트에 대해서 다시 한번 생각해보는 것이 좋을것 같습니다. 감사합니다.
| if(!this.definition[name]) throw new Error(); | ||
| return this.definition[name](target); | ||
| } | ||
| defineMultiple(values){ |
There was a problem hiding this comment.
이부분의 이름도 실제 어떤 부분의 multiple 체크인지 확 느껴지지 않는 메서드이름이네요.
정말 오랜만에 pr 날립니다ㅠㅠ
기능 구현
기능에 따른 정리
개발하면서 느낀점
살펴보시고 피드백 부탁드리겠습니다.
그리고 다른 분들이 데모 올리셨던데 제가 아직 방법을 잘 몰라서 내일 바로 추가하겠습니다. 감사합니다.