Added typings for package

[SlNPacifist]

Typings are true only after https://github.com/apla/node-clickhouse/pull/35

[codecov-io]

Codecov Report

Merging #42 into master will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #42   +/-   ##
=======================================
  Coverage   88.65%   88.65%           
=======================================
  Files           6        6           
  Lines         326      326           
=======================================
  Hits          289      289           
  Misses         37       37

---

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 6b3c26f...391d642. Read the comment docs.

[SlNPacifist]

Oh, you're right. These typings are way too incomplete. I'll update PR later this week. I actually used only "query" part for data insertion and did not bother with other methods. Forgot about that.

[nezed]

Thank you, thats will be quite useful! Please, let me know if i can help you

[SlNPacifist]

Updated.

I tried to leave as much as possible info in comments. Here's what's not described in comments. I had to write down every possible format:

export type Format = "TabSeparated" | "TSV" | "TabSeparatedRaw" ...

This is needed to distinguish return type:

    type DefaultRecordType<ConstructorOptions, QueryOptions, Value> =
        QueryOptions extends {format: "JSON"} ? Record<string, Value> :
        QueryOptions extends {format: "JSONCompact"} ? Array<Value> :
        QueryOptions extends {format: StringResultFormat} ? null :
        ...

If format is string then this conditional does not work: it always boiles down to last type.

DefaultRecordType takes into account most of possible combination of format and dataObjects of both constructor options and query options.

However, such combination does not work:

    const ch = new ClickHouse({ host: "abc", port: 17, dataObjects: true });
    const res = await ch.querying("", { dataObjects: false });
    const x = res.data[0]; // x: Record<string, any>, this is wrong

x type is resolved to Record<string, any>, it should be any[]. I assume this is pretty rare case and fixing typings is not worth it. Fixing this requires much more conditions in DefaultRecordType.

For some reason JSONCompact always returns and array for each record, while JSON returns object. This looks like a bug.

Default record type is defined like this:

RecordType extends ClickHouse.DefaultRecordType<ConstructorOptions, QueryOptions, unknown> = ClickHouse.DefaultRecordType<ConstructorOptions, QueryOptions, any>

unknown type is used to check user-supplied type and any type is used as a default.

We cannot use any type to check user-supplied type for record: [number, string] extends Record<string, any> is true and library user can make an error when refactoring his code.

Nodejs streams are untyped and I left Clickhouse streams untyped (i.e. it accepts any and emits any). This can be fixed by using something more strict like https://github.com/forbesmyester/stronger-typed-streams.

Some usage examples in playground: https://clck.ru/JbpWd

Don't forget to squash before merging if you're OK with these typings.

[SlNPacifist]

@nezed ping

[arikon]

@nezed Will it be merged?

[apla]

Typings is an outdated way to provide type info. Since TypeScript now supports parsing type info from JSDoc comments, it is better to provide typings in JSDoc. Package documentation will be improved too.

[davojan]

@apla that's not true. Writing d.ts is the only way to get quality type definitions. The mentioned JSDoc parsing feature is limited and can't be even close to the hand-crafted typedefs. You can get the idea at least by looking to this definition

Is there any other reason why not to merge this for the community benefit?

[miripiruni]

polite ping