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
base: master
from

Conversation

Projects
None yet
3 participants
@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?

make type definition of load function userfriendly
I know this file is generated, but returning an intersection type is really inconvenient.
@dcodeIO

This comment has been minimized.

Owner

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

This comment has been minimized.

Owner

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

This comment has been minimized.

Owner

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

This comment has been minimized.

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

This comment has been minimized.

Owner

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Owner

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

This comment has been minimized.

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

This comment has been minimized.

Owner

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

This comment has been minimized.

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

This comment has been minimized.

Owner

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

Integrated types fixes into our tsd-jsdoc fork, now using void as ret…
…urn type for undefined, see #527; updated internals and static target to use immutable objects on prototypes
@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Dec 7, 2016

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

@bb

This comment has been minimized.

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 added a commit that referenced this pull request Dec 7, 2016

dcodeIO added a commit that referenced this pull request Dec 7, 2016

@dcodeIO

This comment has been minimized.

Owner

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

This comment has been minimized.

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

This comment has been minimized.

Owner

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

This comment has been minimized.

bb commented Dec 8, 2016

@dcodeIO

This comment has been minimized.

Owner

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.

dcodeIO added a commit that referenced this pull request Dec 8, 2016

dcodeIO added a commit that referenced this pull request Dec 8, 2016

Refactoring; Removed the need for triple-slash references in .d.ts by…
… providing a minimal Long interface, see #527, see #530

@dcodeIO dcodeIO added the bug label Dec 8, 2016

@@ -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;

This comment has been minimized.

@DanielRosenwasser

DanielRosenwasser Dec 9, 2016

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

This comment has been minimized.

@bb

bb Dec 9, 2016

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:

try { fs = eval(['req','uire'].join(''))("fs"); } catch (e) {} // eslint-disable-line no-eval, no-empty
solved it.

This comment has been minimized.

@dcodeIO

dcodeIO Dec 9, 2016

Owner

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.

This comment has been minimized.

@bb

bb Dec 9, 2016

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

This comment has been minimized.

@dcodeIO

dcodeIO Dec 9, 2016

Owner

That sounds like a plan!

This comment has been minimized.

@dcodeIO

@bb bb closed this Dec 9, 2016

@bb bb deleted the bb:patch-1 branch Dec 9, 2016

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