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

Type Definition Files #43

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Type Definition Files #43

wants to merge 6 commits into from

Conversation

phreed
Copy link

@phreed phreed commented Apr 10, 2017

#42

Javascript, being a dynamic language, makes few accommodations for a formal specification.
Two options are generally provided: jsdoc, TypeScript typings.
Integrated development environments provide code-completion (IntelliSense) based on these two specification forms.
This pull request provides TypeScript typings for transit-js.
This does a few things:

extends the specification of the API with exactly those things needed by most code-completion mechanisms
makes using transit-js in TypeScript projects easier
does not interfere with projects that do not care about using types
provides a single place for the formal definition of the API

Caveat:
The typings are not complete.
I was not able to test properly as I was not able to build transit-js.

$ bin/build_release_node
Error: Unable to access jarfile deps/closure-compiler/compiler.jar

@swannodette
Copy link
Contributor

@phreed this looks interesting, I'll get back to you.

@swannodette
Copy link
Contributor

@phreed OK I checked with some folks at Cognitect, we are interested in this - the only caveat being we expect this definitions to be kept up-to-date by the community. The other thing is that we'll need a CA in order to merge this PR.

@phreed
Copy link
Author

phreed commented May 1, 2017

OK
What is the procedure for providing a certificate authority (CA)?
Something like this?
https://help.github.com/articles/signing-commits-using-gpg/

@swannodette
Copy link
Contributor

@phreed sorry by CA, I meant Contributor Agreement. We'll need you to sign this in order to take a PR https://secure.echosign.com/public/hostedForm?formid=8JU33Z7A7JX84U

@phreed
Copy link
Author

phreed commented May 1, 2017

I believe you now have a signed Contributor Agreement from me.
I have a question about what you mean by "kept up-to-date by the community".
Will these updates need to be made as additional PR's where "the community" will be limited to a particular directory?
"The community" is not just limited to myself, correct?

@phreed
Copy link
Author

phreed commented May 1, 2017

I have still not been able to fully test these changes.
The build of transit-js does not work for me.

@swannodette
Copy link
Contributor

@phreed sorry about that I will update the project later this week so myself and others can build properly again. To your earlier question "community" is anyone outside Cognitect and no, there's no reason to partition this and other such contributions into a particular place that I can see at this time. Yes, community contributions can flow in via additional PRs.

@phreed
Copy link
Author

phreed commented May 1, 2017

Sounds good.

@mattbishop
Copy link

@phreed What's the status for this PR? I'd love to see this get in and released this year. I was about to start writing a DefinitelyTyped PR to bridge the gap if necessary.

@swannodette
Copy link
Contributor

FWIW, happy to help push this along. I did update the build scripts.

@swannodette
Copy link
Contributor

@mattbishop actually my bad, @phreed did point out something for me to do in the issue. Need to look into that.

@phreed
Copy link
Author

phreed commented Sep 2, 2017 via email

@mattbishop
Copy link

@phreed what do you mean by "issue with testing"? Are you unclear what acceptable levels of testing are for definition files?

@phreed
Copy link
Author

phreed commented Nov 21, 2017

@mattbishop I could not run the full test suite on my machine. The definition files passed their tests. I communicated with @swannodette on the subject (at the time). It had something to do with an a google closure library that was not present in the node.js context. It has been a while now so the details are a bit hazy.

@mattbishop
Copy link

I'm wondering if these TS definitions are better served over on DefinitelyTyped? I know they prefer the library authors house the definitions but I don't know if that makes sense here given how long it takes to get changes merged. If/when we find a bug in the definition, DefinitelyTyped has a fast change process that rolls out new releases quickly.

@tgriesser
Copy link

Opened up a PR against DefinitelyTyped here to add transit-js, if anyone would like to review: https://github.com/DefinitelyTyped/DefinitelyTyped/pull/60660/files

@tgriesser
Copy link

Types are now available as @types/transit-js: https://www.npmjs.com/package/@types/transit-js

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants