make type definition of load function userfriendly

[bb]

TL;DR

I know this file is generated, but returning an intersection type is really inconvenient.
So please see this more like a discussion starter on how the typing files could be improved.

Description

This PR intentionally modifies a generated file to highlight that using multiple function overload definitions greatly improve user experience, especially when returning different types depending on the input parameters.

before

after

There's still lots of room for improvement beyond this tiny PR, but I wonder what would be a good way for this great project to gain better typings than those generated currently.

/cc @endel - what's your take as one who already contributed TS improvements?

[dcodeIO]

Related: #518

Are you sure this isn't already fixed? For me, Promise<Root> | undefined seems to properly infer a Promise.

[dcodeIO]

Btw: One thought on .d.ts generation I had was to use tripple slash comments or whatnot directly within the JS source to provide the .d.ts types, and then filter that out generating the .d.ts. This way around one could add the definitions inline.

Alternatively, we could patch tsd-jsdoc to output something similar by checking the parameter types and maybe recognizing special jsdoc tags.

Also interesting: http://usejsdoc.org/tags-variation.html

[dcodeIO]

@variation generates something like this:

   /**
    * Loads one or multiple .proto or preprocessed .json files into a common root namespace.
    * @param {string|string[]} filename One or multiple files to load
    * @param {Root|function(?Error, Root=)} [root] Root namespace, defaults to create a new one if omitted.
    * @param {function(?Error, Root=)} [callback] Callback function
    * @returns {Promise<Root>|undefined} A promise if `callback` has been omitted
    * @throws {TypeError} If arguments are invalid
    */
   function load(filename: (string|string[]), root?: (Root|any), callback?: any): (Promise<Root>|undefined);
   
   /**
    * Loads one or multiple .proto or preprocessed .json files into a common root namespace.
    * @name load
    * @function
    * @param {string|string[]} filename One or multiple files to load
    * @param {function(?Error, Root=)} [callback] Callback function
    * @returns {Promise<Root>|undefined} A promise if `callback` has been omitted
    * @throws {TypeError} If arguments are invalid
    * @variation 2
    */
   function load(filename: (string|string[]), callback?: any): (Promise<Root>|undefined);

[bb]

Are you sure this isn't already fixed?

Yeah, you're right. I had the issues with the version on npm and fixed it in my local project's node_modules folder without making sure that the master branch still had this issue. Sorry for the inconvenience.

The variation approach could in addition fix the different signatures. Also, it would be great to get better function signatures than "callback?: any.

I am not sure how far we can go with the triple-slash inlining idea, but I like that.

One thing I just noticed is that each variation needs its own description. In my patch, only the first signature has the description while the others are missing it.

[dcodeIO]

Added a couple of variations for testing and reverted the replacement of () => any to any by using * for class constructors instead.

Does this work for you?

[bb]

Wow, you're quick. Currently seeing the error that the d.ts file is not a module. I do not yet understand why.

[bb]

Now it works again, probably some caching. The signature proposals are working but there are only 3 and I think there should be 4.

The 3rd one has no callback, optional root and returns a promise.

Also, we could explicitly type the callback as

(err: Error, root: Root) => void

I think the information is (more or less) available from the jsdoc comment.

Next, also the class Root would benefit from this improvement.

[dcodeIO]

There is:

With callback: (files, root, callback):undefined
With callback: (files, callback):undefined
Without callback: (files, [root]):Promise

The fourth would be (files, root):Promise but that should be in the 3rd already because root isn't () => any.

[bb]

You're right, again. Thanks for clarification.

[dcodeIO]

Unfortunately, variations do not work with constructors like protobuf.Field (tsd-jsdoc generates multiple classes for these), but it's of course possible to provide the respective parameter alternatives there.

[bb]

I was just skimming https://www.typescriptlang.org/docs/handbook/declaration-files/do-s-and-don-ts.html because I felt unsure about usage of undefined as a return value. It's not as bad as Object or any, but void seems to be used way more often.

https://basarat.gitbooks.io/typescript/content/docs/types/type-system.html#special-types recommends to use void.

[dcodeIO]

Looks like an improvement to tsd-jsdoc, will see what I can do!

[dcodeIO]

This is now integrated into the tsd-jsdoc fork protobuf.js is using!

[bb]

Another issue I'm currently facing is the removal of Long. For my use case, number is just fine, but the typings file references it all the time.

Excerpt:

[at-loader] node_modules\protobufjs\types\protobuf.js.d.ts:2:1
    Cannot find type definition file for 'long'.

[at-loader] node_modules\protobufjs\types\protobuf.js.d.ts:1017:17
    Cannot find name 'Long'.

(at-loader is https://github.com/s-panferov/awesome-typescript-loader which I am using from webpack in my project)

One thing which could be done to reduce it to one might be defining a named type for that:

type ProtoNumber = number|Long

But that doesn't make it go away.

My (temporary?) fix is replacing the reference in the 2nd line with

//// <reference types="long" />
declare type Long = number;

Besides typing, the try-catch require of fs produces a warning by webpack:

WARNING in ./~/protobufjs/src/util.js
Module not found: Error: Can't resolve 'fs' in '[redacted]\node_modules\protobufjs\src'
 @ ./~/protobufjs/src/util.js 103:23-36
 @ ./~/protobufjs/src/index.js

I simply commented this line for now as I am using it in the browser only.

There are more issues like invalid wire types 4 and 7 which I'll check later (and which probably do not belong into this issue).

[dcodeIO]

Not sure if the fix above does the trick for require, though. When webpack doesn't just analyze the source but also runs it, then this could be an issue that can only be solved by a webpack config.

Regarding long: The current setup assumes that the relevant definitions are automatically downloaded when using reference types="..." instead of reference path"...". At least I read about this, but this might be true only when using vscode. Alternatively, we'd have to bundle a (minimal) long.d.ts with protobuf.js and hope that it doesn't conflict with the automatically pulled one. Hm.

[bb]

Commit 299cdea works fine, but 4462d8b produces the following warning:

WARNING in ./~/protobufjs/src/util.js
103:55 Critical dependency: require function is used in a way in which dependencies cannot be statically extracted

So let's use 299cdea, if that is OK for you.

Regarding long: I am using the latest vscode, but it's a typescript project and thus handles types explicitly. It might be the case that this feature is used for JS projects only.

[dcodeIO]

299cdea works because it always throws. Within a new Function context, require is not defined. Will try out something else!

[bb]

pugjs/pug-loader#8 (comment) looks promising

[dcodeIO]

Iirc, there have been issues with providing a webpack config in the past because projects do not inherit these directives from other projects (protobuf.js in this case). If you find out how to do this, let me know.

[DanielRosenwasser]

This doesn't look syntactically correct - is this missing a right paren )?

[bb]

Hi @DanielRosenwasser, yes it is missing. Thanks for your feedback!
Actually the source patch of this PR is kinda outdated as @dcodeIO made great progress on d.ts generation using tsd-jsdoc and the newly generated d.ts does not include my typos.

There was the question how to deal with the optional Long library, but @dcodeIO obviously solved that by providing a minimal interface inside protobuf.js.

The only open point regarding typing I'm seeing right now is that the current d.ts generation does not add callback parameter types, but that's not a TypeScript question, just a missing feature in tsd-jsdoc.

The other issue discussed above more related to webpack than to TypeScript, but that hack: https://github.com/dcodeIO/protobuf.js/blob/7983ee0ba15dc5c1daad82a067616865051848c9/src/util.js#L97 solved it.

[dcodeIO]

Regarding callback parameters: This seems to be related to jsdoc itself. When I tried to find the function signatures in jsdoc's output structures, all I found was function.

[bb]

Maybe you could define a callback type (both in jsdoc and consequently also in .d.ts ) and then just reference that callback in the load signature. It's mentioned here: http://usejsdoc.org/tags-param.html#callback-functions and here: http://usejsdoc.org/tags-callback.html

[dcodeIO]

That sounds like a plan!

[dcodeIO]

47608dd