Started Adding Type Definitions #183
Conversation
Covered All Externals I think. First time writing Type Definitions so anyone else feel free to take over and do a better job.
Removed a Comma that had wound up in a js file when I was first figuring everything out.
|
Thanks for this contribution @SnappyCroissant! |
|
thanks @SnappyCroissant that was a god send, already using the typings on my branch. If I find anything I'll make a PR to you |
|
I've made the typings functional from the initial work from @SnappyCroissant with type safe results from I've made the mistake to place the typings in the working branch though... https://github.com/pocesar/neo4j-javascript-driver |
mv command left over can't use types as variables add some missing stuff, fix implicitAny errors can't use types for vars noImplicitAny and optional parameters type safety for session.run results needs typeof in const declaration for it to work result summary members unprotect members
needs to go inside lib or npm won't install
there's no public accessors in records member
_fields is wrong, it should be an array of T
|
Any reason is has not been merged? |
|
@nbransby lack of time to assess, review and learn about type definitions is the only reason. It is definitely on our list of priorities and this PR will not be lost. |
|
Maybe I can help with that
…On Mon, 24 Apr 2017 at 17:27, Konstantin Lutovich ***@***.***> wrote:
@nbransby <https://github.com/nbransby> lack of time to assess, review
and learn about type definitions is the only reason. It is definitely on
our list of priorities and this PR will not be lost.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#183 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABUobwIcd60zPKds7zlYVDZ1fhQd1z2kks5rzL9hgaJpZM4LXrIc>
.
|
|
@nbransby help would be appreciated. We are currently busy with 1.3 features. I hope to complete them this week. Then there are couple small issues to fix and then I'll try to catch up with type definitions. How would you like to help? Maybe you could review the PR first? At first glance, changes look good but I'm not sure type definitions should live in |
|
Any updates on this? |
|
Not from me, getting by without the definitions as API for basic use is very small, I want to work on this but haven't found the time |
|
Small question to everyone involved. Is it true that with separate type definitions, like in this PR, we will have to manually keep them consistent with actual JS code? I.e. when API is changed developer needs to remember to change/update corresponding type definition? Would it be possible to put type definitions in https://github.com/DefinitelyTyped/DefinitelyTyped? I wonder if we should actually gradually rewrite this driver in TypeScript. |
|
@lutovich this manual process is typical, the benefit of generated type definitions alone doesn't usually suffice as a reason to rewrite a library in typescript. Once completed, the type definitions won't need to be added to DefinitelyTyped because they will be included in the package and picked up by typescript automagically |
|
@nbransby I think TypeScript could be valuable in general, not only for type definitions. Could we put type definitions in DefinitelyTyped only? I.e. forward this PR to DefinitelyTyped and not include it in this driver? What are the benefits of including type definitions in the project directly and not in DefinitelyTyped? |
|
TypeScript is super valuable especially If you come from a strongly typed language background such as Java. The benefits are for the end user - they only have to npm install neo4j-javascript-driver, otherwise they would also need to npm install @types/neo4j-javascript-driver - separately hosted type definitions on DefinitelyTyped exist for libraries where authors arent willing to add definitions to the source. |
|
+1 for this @SnappyCroissant maybe it would be faster if you put it all together and do a PR at |
|
Am I right that the definitions in result-summary.d.ts are outdated? The type |
|
@janwo code was converted to use I will spend some time on type definitions this week. It seems adding type definitions to the driver code base is a good approach. DefinitelyTyped recommends doing it this way and type definitions can serve as additional "documentation" for the code. I'll rebase and cleanup the PR while preserving all current commits. @SnappyCroissant, @pocesar to be able to merge your changes we will need a signed CLA from you. See http://neo4j.com/developer/cla/ for further information. Additional information on contributing code is found at http://neo4j.com/developer/contributing-code/. |
|
@lutovich This needs to be changed in the types as well than. Currently it is referenced as Integer, thus I can call all Integer methods at the moment. |
|
@lutovich sent the CLA email |
|
Been on holiday only got the notification just now. I'll send through the CLA shortly! UPDATE: Sent. |
|
Closing this PR in favour of #251 |
Covered All Externals I think. First time writing Type Definitions so anyone else feel free to take over and do a better job. Also my first PR, so let me know if there's anything I could do better. This will definitely need a few iterations it's far from complete but I need more experienced eyes over it.
There's a trailing comma that wound up in one of the javascript files that I'm about to delete with another commit.