search

apla / node-clickhouse

Yandex ClickHouse driver for nodejs
MIT License
216 stars 50 forks source link

Fix confusing promise interface #18

Closed nezed closed 5 years ago

nezed commented 5 years ago

Иван, спасибо большое за этот драйвер! Он очень хорошо продуман, удобен и функционален!

Problem description:

When we using any promise api, we expect to get some execution result when promise resolved. The good way to use promise interface is, for example, DESCRIBE queries.

ch.querying ("DESCRIBE TABLE system.numbers").then(console.log)
{ rows: 1, statistics: { … }, meta: [ … ], transferred: 377 }

But there is no way to access data.

I've found that syncParser: true option is used for same case it tests

ch.querying ("DESCRIBE TABLE system.numbers", {syncParser: true}).then(console.log)
{ meta: [ … ], data: [ … ], rows: 1, statistics: { … }, transferred: 377 }

So, we've forced to always use syncParser: true option in every .querying call.

Problem conclusion

I thins it was very confusing and not transparent to understand. And this is why i decided to fix this =)

Possible way to improve

I think always use syncParser: true for promise is enough, because there is no difference where to collect buffer with entire query result, inside http.request handler itself or buffer defined in our code.

I've like your notice about promise interface in readme, but i think it must be more convincing. So, i've added "some scary words" to this notice

Fast link to preview: https://github.com/nezed/node-clickhouse/blob/feature/confusing-promise/README.md#promise-interface

codecov-io commented 5 years ago

Codecov Report

Merging #18 into master will increase coverage by 0.02%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #18      +/-   ##
==========================================
+ Coverage   93.19%   93.22%   +0.02%     
==========================================
  Files           5        5              
  Lines         294      295       +1     
==========================================
+ Hits          274      275       +1     
  Misses         20       20
Impacted Files Coverage Δ
src/clickhouse.js 96.81% <100%> (+0.02%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 9c0f14e...b52a703. Read the comment docs.

apla commented 5 years ago

Thank you!

This patch will definitely break some code. Adding another method like consuming with syncParser is much better solution.

nezed commented 5 years ago

@apla Can you please provide more description about which code can be broken? Probably you are talking about end-user interface usage? I think there is no breaking changes in end-user interface or in method behaviour =)

Can you please provide more detailed example about which API will be better for not to break something?

apla commented 5 years ago

Actually, querying is really weird. Stream created, but user have no access to that stream.

nezed commented 5 years ago

Wow, looks like I'm very persuasive 😄 Thank you!)

Yep, probably wired. But logically user expecting from promise same behavior as in stream.on(‘end’, ...)

Promise behavior was very confusing because it was resolved before end event occurs