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

Add an initial typescript definitions file for re-base version 2.8.0 #207

Open
wants to merge 6 commits into
base: 2.x
Choose a base branch
from

Conversation

TheHandsomeCoder
Copy link

  • A test file has been added to test the typings using the examples from the README.md.
  • Added a call to the test script in package.json to compile the test script alongside the karma tests.
  • Add a typings property to package.json

@qwales1
Copy link
Collaborator

qwales1 commented May 16, 2017

@TheHandsomeCoder thanks! going to go through this today.

@TheHandsomeCoder
Copy link
Author

@qwales1 just wanted to address your questions from the issue I submitted and to run something additional past you on it.

Right now I've provided a .tests.ts file which I run through the typescript compiler to ensure that the typings I've added are valid. However on reflection do you think it might be better to instead run the typings test on the karma tests themselves. That way we can ensure that when there are library updates if the tests are passing and compiling then tests using the typings is passing that the typings file is valid.

Also it's possible to add TSDoc to the typings files and get autogenerated documentation from webpack. would this be something you'd be interested in?

@qwales1
Copy link
Collaborator

qwales1 commented May 16, 2017

@TheHandsomeCoder Would adding this work?
https://www.npmjs.com/package/typescript-definition-tester Ideally I was hoping for something like this that could re-run anytime the code changes like the other tests.

@TheHandsomeCoder
Copy link
Author

I'll look into it tomorrow and see can I get it set up. I'll also look to just run the existing tests files through the typescript compiler as that is something that could also give the desired result

@qwales1 qwales1 changed the base branch from master to 2.x May 20, 2017 07:31
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 82cb4e1 on TheHandsomeCoder:master into ** on tylermcginnis:2.x**.

@TheHandsomeCoder
Copy link
Author

I seem to have updated against a different branch? What would be the best branch to redirect this PR to?

@qwales1
Copy link
Collaborator

qwales1 commented Jul 16, 2017

@TheHandsomeCoder I think it should go to 2.x but looks like its maybe rebased on top of the master branch which is 3.x. Typing for 3.x should be a separate PR because the API has changed. It should be easy once its working on 2.x as its just deleting the auth, storage, messaging stuff and I think there is one small change to the syncState options.

@TheHandsomeCoder
Copy link
Author

TheHandsomeCoder commented Jul 16, 2017 via email

@TheHandsomeCoder TheHandsomeCoder changed the title Add an initial typescript definitions file for re-base version 2.7.0 Add an initial typescript definitions file for re-base version 2.8.0 Jul 17, 2017
@TheHandsomeCoder
Copy link
Author

I've reverted the merge from master and pulled from the 2.x branch instead. I've looked through the changes that were made since May 14th and there's nothing currently that I need to change in the typings.

I can however if you'd be interested add the current documentation from the readme to the typings as this can be used for code completion. example attached below.

image

Finally, I looked for a way to automatically test the typings file and I can't for the life of me find anything that does what you're looking for. It would seem that it just doesn't exist in typescript currently. If something is going in that is going to change the signature of the functions, then it needs to be manually updated in the typings.

@qwales1
Copy link
Collaborator

qwales1 commented Jul 21, 2017

@TheHandsomeCoder awesome thanks. Sorry for the delay. Will go through this this weekend for sure.

@christianlacerda
Copy link

Hey @qwales1 , any chance to get this one merged soon?

Thank you!

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