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

Added Typescript declarations. #283

Merged
merged 3 commits into from
Feb 16, 2017
Merged

Added Typescript declarations. #283

merged 3 commits into from
Feb 16, 2017

Conversation

jensvdh
Copy link
Contributor

@jensvdh jensvdh commented Nov 29, 2016

Hi,

I added Typescript definitions to this repository. This should help Typescript developers a lot by providing type declarations for this repository.

See issue #256

Big thanks to @RichiWIP whom provided the first version of a Typescript declaration file.

I decided to follow Microsoft's recommended new guidelines that suggest bundling the Typescript Declarations with the module and adding a reference to package.json. Therefore developers won't have to download the typescript definitions from an external repository.

see:
https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html

Alternatively we can create a second repository and submit our typings to Microsoft's @types project directly. However, this process is more involved and no longer seems recommended.

I added some steps to readme.md to get Typescript developers started. Let me know if you'd like to put this documentation somewhere else.

@jensvdh jensvdh mentioned this pull request Nov 29, 2016
@jensvdh jensvdh changed the title Dev typings support Added Typescript declarations. Nov 29, 2016
@WolframHempel
Copy link
Member

Awesome, thank you very much. We'll review it and merge it shortly

@AlexBHarley AlexBHarley added this to the 2.1 milestone Nov 30, 2016
Copy link
Contributor

@yasserf yasserf left a comment

Choose a reason for hiding this comment

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

This looks awesome!

The only think I could see missing was anonymous records (https://deepstream.io/docs/client-js/datasync-anonymous-record/)

I honestly haven't use typescript before so it may be good for someone else to pitch in and try it out, but it got my thumbs otherwise. If that proves to be hard to find I will make some time early next week to try it out


interface RecordStatic {
/** Get a record. */
getRecord(path: string): Record;
Copy link
Contributor

Choose a reason for hiding this comment

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

getAnonymousRecord is missing here ( as well as an interface )

@yasserf
Copy link
Contributor

yasserf commented Dec 16, 2016

Any chance anyone tested or used this? Be great to get in for 2.1

@jensvdh
Copy link
Contributor Author

jensvdh commented Dec 16, 2016

What's the schedule for 2.1? We noticed a couple of issues that need to be addressed with the typings.

@yasserf
Copy link
Contributor

yasserf commented Dec 16, 2016

We are aiming to get it out by early next week. No rush though, we can easily just merge it in when your happy and do a separate release, our release cycle is mostly automated ;) =D

@yasserf yasserf removed this from the 2.1 milestone Dec 21, 2016
@NathanFlurry
Copy link

I started trying out this declaration file and immediately ran into a few issues. First of all, none of the interfaces or enums are exported, so it's impossible to store values like the Client in TypeScript as a non-any type. Additionally, TypeScript enums are not the same as string enums. For instance, ConnectionState.OPEN would equal 5 based on the declaration file, but in the JavaScript library, it equals the literal string, "OPEN". Therefore, I believe it needs a bit more work. If I get a chance, I'll look into fixing it.

@jensvdh
Copy link
Contributor Author

jensvdh commented Jan 9, 2017

I believe you can use strings in Enums now in Typescript like this:
https://blog.rsuter.com/how-to-implement-an-enum-with-string-values-in-typescript/

@yasserf
Copy link
Contributor

yasserf commented Jan 10, 2017

I'm sorry for the delay on merging this, we are still trying to figure out the best way of testing and maintaining it going forward. It will be merged, just pending the next client milestone.

}
interface deepstreamStatic {
CONSTANTS: {
[key: string]: string
Copy link
Contributor

Choose a reason for hiding this comment

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

In a naive way to use constants, pending 3.0 as said there by @yasserf , CONSTANTS can be changed this way :

CONSTANTS: {
            CONNECTION_STATE: ConnectionState,
            EVENT : Event
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That works. We'll also want to update the enums to use strings. Typescript enums compile to integers.

@yasserf
Copy link
Contributor

yasserf commented Feb 16, 2017

@jensvdh @benjaminVadon Are you both okay with me merging this?

@jensvdh
Copy link
Contributor Author

jensvdh commented Feb 16, 2017

I'm OK with merging this. It's not perfect yet but we can improve on it in a 2nd PR. Have these typings is still an improvement over having none.

@yasserf
Copy link
Contributor

yasserf commented Feb 16, 2017

Agreed!

@yasserf
Copy link
Contributor

yasserf commented Feb 16, 2017

One last thing, would you be okay with signing the CLA? https://deepstream.io/info/community/cla/

I have said that a bit often lately so if you already have my bad!

@jensvdh
Copy link
Contributor Author

jensvdh commented Feb 16, 2017

Done!

@yasserf
Copy link
Contributor

yasserf commented Feb 16, 2017

Awesome, and thanks again for raising this!

@yasserf yasserf merged commit fb58e65 into deepstreamIO:master Feb 16, 2017
mckaydavis added a commit to mckaydavis/deepstream.io-client-js that referenced this pull request Oct 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants