search

feedhenry / fh-sync

Node.js implementation of the FeedHenry Data Synchronisation Server. To be used in conjunction with the FeedHenry Data Synchronisation Client.
http://feedhenry.org
Apache License 2.0
5 stars 17 forks source link

Sync cloud type definitions #8

Closed wtrocki closed 7 years ago

wtrocki commented 7 years ago

ping @paolobueno @nialldonnellyfh

wtrocki commented 7 years ago

I have listed all type definitions with some additional TODO's. Going to check what methods can be used by clients and clean this up.

wtrocki commented 7 years ago

@austincunningham @witmicko - I think that it will be better to host definitions inside fh-sync.

wtrocki commented 7 years ago

Adding changes from upgrade guide: https://github.com/feedhenry/feedhenry-sdk-docs/blob/master/sync_upgrade_guide.adoc

wtrocki commented 7 years ago

As single review for this task may be really time consuming best to split reviews into sections and list what sections were covered in the comment.

wtrocki commented 7 years ago

@aidenkeating Should we also document all methods. etc ? WDYT? Thanks a lot for review.

aidenkeating commented 7 years ago

@wtrocki I think documenting the methods here is a good idea. I think it could be another ticket though as I think this PR has a considerable amount of work in it already, up to you though :)

We have a good amount of JSDoc written in fh-sync, however we are missing/not using some useful features such as defining default parameters for variables (we have them documented, just not using JSDoc syntax) etc.

Feel free to ping me on the documentation work too 👍

wtrocki commented 7 years ago

@wei-lee can you quickly take look to make sure that we do not have any critical issues here :) Definition file will be maintained by RainCatcher team and will be useful for generating top level documentation for sync.

aidenkeating commented 7 years ago

@wtrocki @wei-lee I was thinking about the idea of testing/maintaining compatibility of the declaration file with the JavaScript implementation in the longer term.

Perhaps the best way to ensure the TypeScript declaration file is always in sync with the JavaScript implementation would be to write the tests for this repo in TypeScript. This would ensure that they are always aligned (interface-wise) as the tests would fail if they weren't.

I think this would result in the RainCatcher team not having to be so attentive to the declaration file (leaving other contributors to forget about the TypeScript part), instead the maintenance of it would be shared among anyone who makes a PR.

Any thoughts on this?

aidenkeating commented 7 years ago

@wtrocki I can't seem to get the declaration file working. Have you tried implementing this in a TypeScript project?

wtrocki commented 7 years ago

@aidenkeating - Doing verification now.

wtrocki commented 7 years ago

@aidenkeating Great idea. We can port one of the unit tests to be running in TypeScript

wei-lee commented 7 years ago

@wtrocki @aidenkeating that sounds good. Another option is that we could start converting fh-sync to use typescript, and generate the definition file from the typescript files directly. In the longer term, I think it makes sense to start using typescript for this module.

wtrocki commented 7 years ago

Fixed problem with namespace. Addressed additional comments.

wtrocki commented 7 years ago

Verified using sync example application.