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.

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

Support node v6 #1

Merged
merged 2 commits into from
May 21, 2018
Merged

Support node v6 #1

merged 2 commits into from
May 21, 2018

Conversation

kanziw
Copy link
Contributor

@kanziw kanziw commented May 21, 2018

Production node 버전을 6점대로 사용하고 있습니다.
v8 이상 부터 async / await 및 util/promisify 를 지원하기 때문에 v6 에선 사용이 안되더라구요.

원래는 더 낮은 버전도 지원 가능하도록 수정하고 싶었으나, 사용하고 계신 meow 모듈이 v6 이상부터 지원하길래 hosejs 도 v6 까진 지원하도록 수정을 해보았습니다.

v0.12.0 에서의 npm i 로그 첨부합니다.

npm WARN engine meow@5.0.0: wanted: {"node":">=6"} (current: {"node":"0.12.0","npm":"2.5.1"})
npm WARN engine get-stdin@6.0.0: wanted: {"node":">=4"} (current: {"node":"0.12.0","npm":"2.5.1"})
npm WARN engine redent@2.0.0: wanted: {"node":">=4"} (current: {"node":"0.12.0","npm":"2.5.1"})
npm WARN engine minimist-options@3.0.2: wanted: {"node":">= 4"} (current: {"node":"0.12.0","npm":"2.5.1"})
npm WARN engine camelcase-keys@4.2.0: wanted: {"node":">=4"} (current: {"node":"0.12.0","npm":"2.5.1"})
npm WARN engine read-pkg-up@3.0.0: wanted: {"node":">=4"} (current: {"node":"0.12.0","npm":"2.5.1"})
npm WARN engine trim-newlines@2.0.0: wanted: {"node":">=4"} (current: {"node":"0.12.0","npm":"2.5.1"})
npm WARN engine read-pkg@3.0.0: wanted: {"node":">=4"} (current: {"node":"0.12.0","npm":"2.5.1"})
npm WARN engine find-up@2.1.0: wanted: {"node":">=4"} (current: {"node":"0.12.0","npm":"2.5.1"})
npm WARN engine camelcase@4.1.0: wanted: {"node":">=4"} (current: {"node":"0.12.0","npm":"2.5.1"})
npm WARN engine indent-string@3.2.0: wanted: {"node":">=4"} (current: {"node":"0.12.0","npm":"2.5.1"})
npm WARN engine strip-indent@2.0.0: wanted: {"node":">=4"} (current: {"node":"0.12.0","npm":"2.5.1"})
npm WARN engine map-obj@2.0.0: wanted: {"node":">=4"} (current: {"node":"0.12.0","npm":"2.5.1"})
npm WARN engine camelcase@4.1.0: wanted: {"node":">=4"} (current: {"node":"0.12.0","npm":"2.5.1"})
npm WARN engine quick-lru@1.1.0: wanted: {"node":">=4"} (current: {"node":"0.12.0","npm":"2.5.1"})
npm WARN engine hosted-git-info@2.6.0: wanted: {"node":">=4"} (current: {"node":"0.12.0","npm":"2.5.1"})
npm WARN engine locate-path@2.0.0: wanted: {"node":">=4"} (current: {"node":"0.12.0","npm":"2.5.1"})
npm WARN engine path-type@3.0.0: wanted: {"node":">=4"} (current: {"node":"0.12.0","npm":"2.5.1"})
npm WARN engine load-json-file@4.0.0: wanted: {"node":">=4"} (current: {"node":"0.12.0","npm":"2.5.1"})
npm WARN engine p-locate@2.0.0: wanted: {"node":">=4"} (current: {"node":"0.12.0","npm":"2.5.1"})
npm WARN engine path-exists@3.0.0: wanted: {"node":">=4"} (current: {"node":"0.12.0","npm":"2.5.1"})
npm WARN engine strip-bom@3.0.0: wanted: {"node":">=4"} (current: {"node":"0.12.0","npm":"2.5.1"})
npm WARN engine parse-json@4.0.0: wanted: {"node":">=4"} (current: {"node":"0.12.0","npm":"2.5.1"})
npm WARN engine pify@3.0.0: wanted: {"node":">=4"} (current: {"node":"0.12.0","npm":"2.5.1"})
npm WARN engine pify@3.0.0: wanted: {"node":">=4"} (current: {"node":"0.12.0","npm":"2.5.1"})
npm WARN engine p-limit@1.2.0: wanted: {"node":">=4"} (current: {"node":"0.12.0","npm":"2.5.1"})
npm WARN engine p-try@1.0.0: wanted: {"node":">=4"} (current: {"node":"0.12.0","npm":"2.5.1"})

Copy link
Owner

@deptno deptno left a comment

Choose a reason for hiding this comment

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

첫 번째 PR이군요 🎉 감사합니다. 제가 테스트 환경을 만들어 두질 않아서 눈으로만 리뷰를 해봤습니다. 의견 달아주시면 그에 따라 머지할게요. 감사합니다.

index.ts Outdated
@@ -49,6 +47,14 @@ async function main() {
}
}

async function readFile (...args) {
return new Promise((resolve, reject) => {
fs.readFile.call(fs, ...args, (err, data) => {
Copy link
Owner

Choose a reason for hiding this comment

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

여기서 call 이 필요한가요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://nodejs.org/api/fs.html#fs_fs_readfile_path_options_callback

레퍼런스 문서를 보면 fs.readFile 은 인자를 3개를 받습니다. general 하게 option 도 받게 하기 위해 call 을 사용하였습니다.

사실은...

image

위와 같은 이유로 call 로 우회하긴 했습니다.ㅎㅎ

typescript 론 위 상황을 어떻게 핸들링 하는게 좋으신지 의견을 여쭙고 싶습니다.

아래처럼 명시적으로 사용하기엔 options 값이 <Object> | <string> 이라 좀 난해하네요.

image

Copy link
Owner

Choose a reason for hiding this comment

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

아 현실적인 이유였군요

제생각은 general 하지만 type을 상실하게 되므로 명시적으로 아래 붙여주신 바와 같이

function readFile(path: string, options?) {
    return new Promise ....
}

호출하는 쪽에서 타입을 알 수 있는게 타입스크립트 way가 아닐까 합니다.

추가적으로 es5 타겟으로 컴파일을 하게 되면 앞에 async 키워드가 붙게되면 컴파일 되어 떨어지는 파일 생성이 다르게 생성이 되는데 async 를 제외할때 native Promise의 사용이 이루어 지는데 이 경우(async / await 세트가 아닌)는 그쪽 이 더 낫지 않나 생각됩니다. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

아직 typescript 가 익숙치 않아 typescript 스럽지 못한 코드가 나오게 되었네요. 덕분에 조금 더 typesscript 다운게 무엇인지 생각할 수 있었습니다.
또한 async 를 불필요하게 사용했었네요. 좋은 코드 리뷰 감사합니다 :)

Copy link
Owner

Choose a reason for hiding this comment

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

es2017 이상에서는 무관합니다. 저야말로 버전 분석에서 많이 배웠습니다. 릴리즈는 CI 달고 하도록하겠습니다. PR 감사합니다!

@deptno deptno merged commit 5856e2a into deptno:master May 21, 2018
@kanziw kanziw deleted the support-node-v-6 branch May 21, 2018 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants