Pg-promise: TypeScript typings have multiple problems

[paavohuhtala]

Hey,

Thanks for implementing TypeScript type definitions for your library. Even though they are very new, they are much more complete than the ones available from DefinitelyTyped.

However, the current version has a few problems that limit their usability.

• Most importantly, I haven't got them to work yet. I get the following error (which I haven't researched further) when trying to reference them:

/node_modules/pg-promise/typescript/pg.ts(136,5): error TS2309: An export assignment cannot be used in a module with other exported elements.

• The previous error would probably be solved by using the pg.d.ts from DefinitelyTyped. It's widely used, and well maintained. The usual way is to just import ... from "pg" in your type definitions, and let the user supply the type definitions for pg via typings. Using your library via TypeScript would require typings, but most things in the TypeScript world do.
• You have to manually reference the typings, either using tsconfig.json or via <reference> in a source file. This is prone to breaking (when refactoring) and generally not very nice.
  
  
  
  • To fix this you could include a property named typings in the package.json pointing to the typings file, so the TypeScript compiler can discover the typings automatically.
  
  
  • Your typings should be concatenated into a single file, with the ending .d.ts. Additionally, the currently included .js files in the typescript directory are not required.
  
  
  • Also, the typings shouldn't include any `/// <reference>s.
  
  
  • This article tells more about it
  
  
  • Alternatively, you could contribute your typings to DefinitelyTyped and/or the typings registry; I'm not sure about the process.
  
  
• You should consider exporting some/all of the interfaces, so you could import them directly without having to use * as.
• Some functions could be typed a bit better. For instance, the query methods in IBaseProtocol should take string | QueryFile | IPreparedStatement instead of any (you have to write an interface for the last one).
• I really don't like the fact you have to edit a file in the module to change the type of the promise [provider]. You might be able to implement it better via generics, by having a type parameter TPromise extends Promise<T>. However, that might be too verbose to implement and/or use. Default type variables might help, but they aren't even planned yet for the language.
• Small things
  
  
  
  • Instead of Array<T>, you can use T[].
  
  
  • You might want to use PascalCase for enums, as per the official TypeScript coding conventions.
  
  

[vitaly-t]

Writing TypeScript for pg-promise was my first TypeScript project ever. I was looking for contributors, but nobody wanted to help, so in the end I had to finish it all up by myself.

Thank you for taking time writing it all in detail, I will try to answer it item-by-item...

First, I am surprised you couldn't get it to work, because not only the library itself has lots of tests included (folder test\typescript), but pg-promise-demo uses it entirely, and no problem. Check it out, let me know if you have any problems running that demo.

Are you using version 3.8.1 of pg-promise? If not, then it would be of no surprise, you must use that one ;)

The issue with pg.d.ts, its version on DefinitelyTyped is way more incomplete than mine. And as stated at the top of my version:

// Declaring only the part of the 'pg' module that's useful within pg-promise

so I can't really use that one, I have to use my own version, that's why I wrote and included it.

Next one...

You have to manually reference the typings, either using tsconfig.json or via in a source file.

That's because this library doesn't come from Typings, it is distributed with pg-promise. Maybe someday it will.

This is prone to breaking (when refactoring) and generally not very nice.

How would you break it? I can't think of one way. The path with node_modules will remain the same always.

You should consider exporting some/all of the interfaces, so you could import them directly without having to use * as.

One of the ideas around ambient declarations is to replicate the existing interface precisely. When you start exporting things that are not available in the original library you are deviating from the official protocol, which isn't very nice.

Some functions could be typed a bit better. For instance, the query methods in IBaseProtocol should take string | QueryFile | IPreparedStatement instead of any (you have to write an interface for the last one).

For one thing, there is no PreparedStatement in the library. Instead, there is only direct support for Prepared Statements, using {name, text, values}. Basically, there are so many variations, I simplified it for the time being. Possible to improve in the future.

I really don't like the fact you have to edit a file in the module to change the type of the promise [provider]. You might be able to implement it better via generics, by having a type parameter TPromise extends Promise...

None of that would work, because we are supplying promise library dynamically, and it can be any promise library. You cannot extend a generic in TypeScript, I have researched this subject in detail, it is still in discussion to be implemented in the future, but until that happens, you have to patch the library with explicit promise type, or else you won't have access to its extended protocol.

Instead of Array, you can use T[]

How is it any better?

You might want to use PascalCase for enums, as per the official TypeScript coding conventions.

The cases used for enum-s are exactly the same as in the original library. Changing them would break the protocol.

[vitaly-t]

@paavohuhtala

I would like to be able to address each issue in detail. There is way too much in both of our posts here.

I would suggest opening separate issues for each problem, or else it becomes too difficult to follow, especially for other developers.

Please don't hesitate opening separate issues for each of the problems discussed here.

[paavohuhtala]

Thanks for your feedback. I don't have the time to write a full response right now, but I can confirm that I've managed to get your typings to work :+1:. It was caused by a stupid mistake on my part; the node-postgres declerations from typings conflicted with the ones in your library.

I'll review my issues when I have the time, and promote them to full issues when applicable.

[vitaly-t]

@paavohuhtala thank you, will look forward.

As for the promises, not to let you think that I haven't tried, a spent a lot of time trying to make it work without using the patch approach, but couldn't get it to work, always running into issues on the declarative level.

Here's where I got stuck: http://stackoverflow.com/questions/36593087/using-a-custom-promise-as-a-generic-type

[vitaly-t]

@paavohuhtala if you follow on that StackOverflow issue, it pretty much came to the expected conclusion, from some experts in that area, confirming that it is not possible in today's TypeScript.

http://stackoverflow.com/questions/36593087/using-a-custom-promise-as-a-generic-type

It seems that the patching thing is the only one currently usable to inject a custom promise into the library's protocol.

[vitaly-t]

@paavohuhtala In version 3.8.2 I renamed modules pg and promise into pg-subset and ext-promise, to avoid a conflict in case you already have a module with that name used somewhere.

[vitaly-t]

@paavohuhtala will you get a chance to follow up with separate issues added or should we just close the issue?

[paavohuhtala]

You can close this issue for now. The most important thing you should do is to follow this guide so that the TypeScript compiler can discover the typings for the library automatically.

I'll open more issues and/or pull requests should the need arise.

[vitaly-t]

@paavohuhtala those guidelines only apply to writing a TypeScript library, they do not apply to writing ambient declarations, it is a totally different story, because one is bound to the protocol implemented by the underlying library.

[paavohuhtala]

????

The link is about adding a property to package.json, so that the compiler can discover the typings by itself. It's not about naming conventions or anything related to the protocol. Additionally, I don't think it's limited to TypeScript projects; any project with included typings should work.

Also the typings should use the file ending d.ts. Currently, if you include the typings using tsconfig.json, the compiler will compile the typings into .js-files, instead of just using them for type resolution. This will result in unnecessary files.

[vitaly-t]

I did use .d.ts initially, but then renamed, because I couldn't find out what was that for... :)

[vitaly-t]

@paavohuhtala there has been a number of improvements in TypeScript in 3.9.0, including:

• Renamed all files back to contain .d.ts in the name
• Strict query parameter for all query methods (as you asked earlier)
• Support for the new Prepared Statements.

See 3.9.0 release notes ;)