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

make type definition of load function userfriendly #527

Closed
wants to merge 1 commit into from

Conversation

bb
Copy link

@bb bb commented Dec 7, 2016

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

original
returning-object

after

listing-different-signatures
autocomplete-promise

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?

I know this file is generated, but returning an intersection type is really inconvenient.
@dcodeIO
Copy link
Member

dcodeIO commented Dec 7, 2016

Related: #518

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

@dcodeIO
Copy link
Member

dcodeIO commented Dec 7, 2016

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
Copy link
Member

dcodeIO commented Dec 7, 2016

@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
Copy link
Author

bb commented Dec 7, 2016

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 commit that referenced this pull request Dec 7, 2016
@dcodeIO
Copy link
Member

dcodeIO commented Dec 7, 2016

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
Copy link
Author

bb commented Dec 7, 2016

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

@bb
Copy link
Author

bb commented Dec 7, 2016

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
Copy link
Member

dcodeIO commented Dec 7, 2016

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
Copy link
Author

bb commented Dec 7, 2016

You're right, again. Thanks for clarification.

dcodeIO added a commit that referenced this pull request Dec 7, 2016
@dcodeIO
Copy link
Member

dcodeIO commented Dec 7, 2016

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
Copy link
Author

bb commented Dec 7, 2016

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
Copy link
Member

dcodeIO commented Dec 7, 2016

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

dcodeIO added a commit that referenced this pull request Dec 7, 2016
…urn type for undefined, see #527; updated internals and static target to use immutable objects on prototypes
@dcodeIO
Copy link
Member

dcodeIO commented Dec 7, 2016

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

@bb
Copy link
Author

bb commented Dec 7, 2016

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
Copy link
Member

dcodeIO commented Dec 8, 2016

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
Copy link
Author

bb commented Dec 8, 2016

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
Copy link
Member

dcodeIO commented Dec 8, 2016

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

@bb
Copy link
Author

bb commented Dec 8, 2016

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

@dcodeIO
Copy link
Member

dcodeIO commented Dec 8, 2016

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.

@@ -1194,7 +1197,8 @@ declare module "protobufjs" {
* @returns {Promise<Root>|undefined} A promise if `callback` has been omitted
* @throws {TypeError} If arguments are invalid
*/
load(filename: (string|string[]), callback?: any): (Promise<Root>|undefined);
load(filename: (string|string[])): Promise<Root>;
load(filename: (string|string[]), callback: (err: any, root: Root): void;

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

That sounds like a plan!

Copy link
Member

Choose a reason for hiding this comment

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

@bb bb closed this Dec 9, 2016
@bb bb deleted the patch-1 branch December 9, 2016 08:53
dcodeIO added a commit that referenced this pull request Dec 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants