Update of typescript definitions #175

reuzel · 2018-05-11T00:31:15Z

Hi,

I took the liberty to update the typescript definitions. They were not working for me, so I corrected them as far as I could.

I tried to use generics on the types, but reverted those changes for now as these become awkward with the field definitions in the types (not sure how to create an array of generic field types, where the generic parameter is not set yet...).

There may be more TODOs but, I think this one is an improvement...

Regards,
Joost

coveralls · 2018-05-11T00:33:11Z

Coverage remained the same at 99.976% when pulling ce38a9a on reuzel:master into 8293a6b on mtth:master.

mtth

Thank you for the PR @reuzel! Can you describe the issues you are having with the current typings?

mtth · 2018-05-14T04:35:58Z

types/index.d.ts

-type Schema = string | object;  // TODO object should be further specified
+//"virtual" namespace (no JS, just types) for Avro Schema
+declare namespace schema
+{


Can you put the opening brace on the same line as the declaration for consistency with the rest of the file? (Throughout too.)

ah, styling. Will do.

mtth · 2018-05-14T04:47:10Z

types/index.d.ts

+    fields: {
+          name: string;
+          doc?:string;
+          type: AvroSchema;


All nested AvroSchemas could be relaxed to Schema (Types are also accepted).

Schema lives in a namespace outside this one (one level up). Would that not violate layering? I thought to make this namespace completely independent on how it is being used. It is just a typescript representation of the Avro spec. Or am I misunderstanding you?

It's sometimes convenient to nest types inside of a schema declaration (a simpler alternative to passing them through Type.forSchema's registry option). I'm not sure whether the namespacing is worth the extra restriction; it might also break existing code.

Will change.

mtth · 2018-05-14T04:51:44Z

types/index.d.ts

+//"virtual" namespace (no JS, just types) for Avro Schema
+declare namespace schema
+{
+  export type AvroSchema = DefinedType | DefinedType[];


Does this allow references? (References can be arbitrary strings, pointing to values in a registry.)

ooh, perhaps not. I haven't used references in my project so far. It would be easy to fix, to add a string type to the DefinedType (along with the primitive, complex and logical types). The primitive type than becomes a kind of redundant as they are just a collection of strings, though it still helps in code-completion scenarios.

Sounds good.

reuzel · 2018-05-14T07:18:16Z

The issues I had with the typings where mainly the following:

the Blob support was missing
As I'm using your library to serialize objects before storing them in a DB, I wanted to use the toBuffer and fromBuffer, but those typings were incorrect.
My project streams a large resultset as an Avro encoded and compressed file towards clients. To this end I want to stream the objects to an http result. However, none of the streams in typings inherited from stream.Duplex, making them unusable in most regular streaming scenarios. For example, I couldn't simply pipe to/from them.

Less important:

I missed the Avro schema typings (made some mistakes there)
All the options were untyped, so I always had to search on the wiki for the exact naming. I was looking for some code-completion there.

jsphweid · 2018-05-14T21:31:14Z

Also, should support for .on('finish', () => etc.) be added?

adding support for referenced types, adding events to BlockDecoder definitions

Update based on pull request mtth/avsc mtth#178

mtth

Apologies for the slow review; I was travelling this past week. Mostly just one comment about duplex events.

mtth · 2018-05-20T22:58:53Z

types/index.d.ts

+//"virtual" namespace (no JS, just types) for Avro Schema
+declare namespace schema
+{
+  export type AvroSchema = DefinedType | DefinedType[];


Sounds good.

mtth · 2018-05-20T23:07:05Z

types/index.d.ts

+    fields: {
+          name: string;
+          doc?:string;
+          type: AvroSchema;


It's sometimes convenient to nest types inside of a schema declaration (a simpler alternative to passing them through Type.forSchema's registry option). I'm not sure whether the namespacing is worth the extra restriction; it might also break existing code.

mtth · 2018-05-20T23:08:34Z

types/index.d.ts

+    constructor(opts?: Partial<DecoderOptions>);
+    static defaultCodecs(): CodecOptions;
+
+    //adding all listeners, etc. Can't add a single method override for metadata, all base methods need to be repeated :-(


Are these really required? How come they aren't provided by Duplex?

The implementation of most of these is provided by Duplex. However, in the definition files, Typescipt requires you to specify the overload function (the one with string, (...args:any[]) ). Once you do that, all the events from the base-class (Duplex) are hidden from code completion (at least in vscode). To make them visible again, you have to add them all :-(

I see this as a Typescript or otherwise vscode bug...

That's a bummer. It might be surprising to add them here but not to the other duplex streams below; I'd prefer staying consistent and not adding them anywhere, at the cost of losing auto-completion (until the issue you mention is fixed) - do you mind you take them out? (Adding them everywhere would be an unreasonable amount of clutter for the benefits.)

mtth · 2018-05-20T23:09:58Z

types/index.d.ts

-  fromString(str: any): any;
+  createResolver(type: Type): Resolver;
+  decode(buf: Buffer, pos?: number, resolver?: Resolver): { value: any, offset: number};
+  encode(val: any, buf: Buffer, pos?: number): void;


encode returns a number (the new offset in the buffer).

mtth

One last change and I expect this will be ready to merge.

mtth · 2018-05-26T15:52:14Z

types/index.d.ts

+    constructor(opts?: Partial<DecoderOptions>);
+    static defaultCodecs(): CodecOptions;
+
+    //adding all listeners, etc. Can't add a single method override for metadata, all base methods need to be repeated :-(


That's a bummer. It might be surprising to add them here but not to the other duplex streams below; I'd prefer staying consistent and not adding them anywhere, at the cost of losing auto-completion (until the issue you mention is fixed) - do you mind you take them out? (Adding them everywhere would be an unreasonable amount of clutter for the benefits.)

alexander-alvarez · 2018-06-01T18:29:49Z

what's the status here? would like to see the types updates wrapped up (or reverted) and released if possible

reuzel · 2018-06-01T19:05:53Z

Sorry, but I’ve been extremely busy lately. I will come back to this as soon as I can. Expect the changes to be made within a few days from now. Regards, Joost

reuzel · 2018-06-03T11:56:13Z

Last question, before making the changes: The BlockDecoder is the only duplex stream where the addition of all the events is necessary, as it is the only one that adds an event. The repetition of events is only necessary when an extension is made. The event override 'hides' the events from the super classes (as far as those exist in JavaScript). As the BlockDecoder is the only one that emits a new type of event, it is the only one for which the repetition is required. Given that, there are thee options:

Remove all the listeners: the metadata event will work type-check wise but is not visible in any code-completion. The Duplex events will be visible.
Add the metadata event, hide all the standard Duplex events.
Add the meta-data event and repeat the definition of all the standard duplex events (as in the current PR).

None of them are as elegant as you would wish, but I leave the choice to you...

Regards,
Joost

mtth · 2018-06-04T01:32:48Z

Thank you for the detailed explanation @reuzel, this makes sense. Can we go with the first option, adding a comment linking to this PR mentioning why the metadata event is not declared? It's unfortunate that code-completion will not display the event, but the benefits outweigh the inconvenience IMHO.

reuzel · 2018-06-04T22:30:24Z

@mtth The PR should now be in a state that it can be merged.

alexander-alvarez · 2018-06-04T22:36:56Z

@reuzel worth noting you can do a repository pattern like so https://github.com/Blizzard/node-rdkafka/pull/420/files#diff-b52768974e6bc0faccb7d4b75b162c99R21
so at least the clutter moves somewhere else?

/shrug

mtth · 2018-06-05T03:00:09Z

Merged; thank you @reuzel!

alexander-alvarez · 2018-06-07T20:42:50Z

types/index.d.ts

+export function createFileDecoder(fileName: string, opts?: Partial<DecoderOptions>): streams.BlockDecoder;
+export function createFileEncoder(filePath: string, schema: Schema, opts?: Partial<EncoderOptions>): streams.BlockEncoder;
+export function createBlobEncoder(schema: Schema, opts?: Partial<EncoderOptions>): stream.Duplex;
+export function createBlobDecoder(blob: Blob, opts?: Partial<DecoderOptions>): streams.BlockDecoder;


FWIW I had to add dom as a lib to my tsconfig.json for typescript to compile O.K. after this change.
Not sure if this is something we can work around though in this file. @reuzel

"lib": ["dom"]

I think a triple slash directive should make it work, but it's not taking effect on my local
/// <reference types="dom" />

The triple slash syntax is rather outdated. I never use it anymore. In this case, the lib option seems the right way forward. It is just telling the typescript compiler that the dom objects are assumed to be available. Without, the Blob type would be unknown.

Why do you see this as an issue? As far as I know, the lib option is not transitive, in the sense that clients of your library do not need to add them as long as they do not include the Blob encoders/decoders. As you have nicely splitted that functionality into a browser lib, I don't really see an issue?

By the way, I didn't encounter this issue in my own project. I'm using webpack for the client pieces, and when set in the right mode, WebPack enables the dom objects automatically.

I forgot to mention I'm running in node (node doesn't have dom). So it's counter-intuitive from a consumer's standpoint to have to enable the dom types when consuming node.js libraries, when those dom types don't exist.

I think the longer term solution which I might have seen in some comments is to have different declaration files for the different build assets?

Update of typescript definitions

9387fd6

Adding LogicalType to schema

8eba602

mtth reviewed May 14, 2018

View reviewed changes

reuzel added 2 commits May 15, 2018 01:12

styling.

89af707

adding support for referenced types, adding events to BlockDecoder definitions

Fixing decode, equals and toJSON

ce38a9a

Update based on pull request mtth/avsc mtth#178

mtth mentioned this pull request May 20, 2018

types: More type defintions #178

Closed

mtth reviewed May 20, 2018

View reviewed changes

relaxing Schema's to include type, fix of encode return value

126bfd0

mtth reviewed May 26, 2018

View reviewed changes

removing events from BlockDecoder to make definition file more readable.

c4164a7

mtth merged commit c4164a7 into mtth:master Jun 5, 2018

alexander-alvarez reviewed Jun 7, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update of typescript definitions #175

Update of typescript definitions #175

reuzel commented May 11, 2018

coveralls commented May 11, 2018 •

edited

mtth left a comment

mtth May 14, 2018

reuzel May 14, 2018

mtth May 14, 2018

reuzel May 14, 2018

mtth May 20, 2018

reuzel May 23, 2018

mtth May 14, 2018

reuzel May 14, 2018

mtth May 20, 2018

reuzel commented May 14, 2018

jsphweid commented May 14, 2018

mtth left a comment

mtth May 20, 2018

mtth May 20, 2018

mtth May 20, 2018

reuzel May 23, 2018

mtth May 26, 2018

mtth May 20, 2018

mtth left a comment

mtth May 26, 2018

alexander-alvarez commented Jun 1, 2018

reuzel commented Jun 1, 2018 via email •

edited

reuzel commented Jun 3, 2018 •

edited

mtth commented Jun 4, 2018

reuzel commented Jun 4, 2018

alexander-alvarez commented Jun 4, 2018

mtth commented Jun 5, 2018

alexander-alvarez Jun 7, 2018

alexander-alvarez Jun 7, 2018

reuzel Jun 8, 2018

alexander-alvarez Jun 8, 2018

Update of typescript definitions #175

Update of typescript definitions #175

Conversation

reuzel commented May 11, 2018

coveralls commented May 11, 2018 • edited

mtth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reuzel commented May 14, 2018

jsphweid commented May 14, 2018

mtth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-alvarez commented Jun 1, 2018

reuzel commented Jun 1, 2018 via email • edited

reuzel commented Jun 3, 2018 • edited

mtth commented Jun 4, 2018

reuzel commented Jun 4, 2018

alexander-alvarez commented Jun 4, 2018

mtth commented Jun 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 11, 2018 •

edited

reuzel commented Jun 1, 2018 via email •

edited

reuzel commented Jun 3, 2018 •

edited