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.

Sign up for GitHub

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

Jump to bottom

make type definition of load function userfriendly #527

Closed
wants to merge 1 commit into from
Closed

make type definition of load function userfriendly #527

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions types/protobuf.js.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,10 @@ declare module "protobufjs" {
* @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);
function load(filename: (string|string[])): Promise<Root>;
function load(filename: (string|string[]), callback: (err: any, root: Root) => any): Object;
function load(filename: (string|string[]), root: Root): Promise<Root>;
function load(filename: (string|string[]), root: Root, callback: (err: any, root: Root) => any): Object;

/**
* Options passed to {@link inherits}, modifying its behavior.
Expand Down Expand Up @@ -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;
Copy link

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.

47608dd


}

Expand Down