Skip to content
This repository was archived by the owner on Jun 13, 2019. It is now read-only.

Typescript #119

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

Typescript #119

wants to merge 5 commits into from

Conversation

hansmbakker
Copy link
Contributor

I started working on typescript definitions for iotivity-node.
The strong typing prevents mistakes and assists during programming with intellisense in VS Code.

This is a work in progress, because I'm basically reverse engineering the JavaScript which is not always clear (e.g. objects are extended in several places with lodash, so that you don't have a clear overview of an object's properties, sometimes the return types of methods are not clear).

I yet have to start the lowlevel typings.

I could use some help and feedback from somebody who has more experience with programming for iotivity-node. Also I'm interested in hearing the rationale behind the code style (lots of lodash use and code nesting).

Is there anybody who would like to give me feedback or help a hand?

@zolkis
Copy link
Contributor

zolkis commented Feb 12, 2017

Actually this is something we've been thinking about, and will consider adding TypeScript definitions to the spec. I hope to update on this during the coming days.

For time being, we can use an issue in this repo to discuss unclear things. So please check the spec, and point out where should it be more clear.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 87.083% when pulling e1c08f1 on wind-rider:typings into ab0e6f3 on otcshare:master.

@hansmbakker
Copy link
Contributor Author

@zolkis thank you for linking to that spec, it helps a lot! I didn't know that iotivity-node was an implementation of that spec so it serves as good documentation!

I found out that the spec seems not to be followed wrt. the Error types - the spec uses Error subclasses (https://github.com/01org/iot-js-api/tree/master/api/ocf#error-handling) while iotivity-node uses the Error base class for everything, I'll make an issue for that

@coveralls
Copy link

Coverage Status

Coverage remained the same at 87.083% when pulling 82624cd on wind-rider:typings into ab0e6f3 on otcshare:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 87.083% when pulling 9c4f92b on wind-rider:typings into ab0e6f3 on otcshare:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 87.083% when pulling 9c4f92b on wind-rider:typings into ab0e6f3 on otcshare:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 87.083% when pulling 2231e0c on wind-rider:typings into a4c1fd3 on otcshare:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 87.083% when pulling 2231e0c on wind-rider:typings into a4c1fd3 on otcshare:master.

@hansmbakker
Copy link
Contributor Author

@zolkis, @gabrielschulhof do you have any feedback?

@zolkis
Copy link
Contributor

zolkis commented Feb 23, 2017

Give it some more time, ELC is ongoing :)

@hansmbakker
Copy link
Contributor Author

Sorry, I didn't know that

@gabrielschulhof
Copy link

@Wind-rider Sorry about the delay!

The change looks good overall, however, I am concerned that it adds a lot of weight to the installed size of this package, because it pulls type definitions for all of lodash.

Would it be possible to create a new npm package which provides these definitions and depends on iotivity-node?

@hansmbakker
Copy link
Contributor Author

@gabrielschulhof it is possible to add something to https://github.com/DefinitelyTyped/DefinitelyTyped/ but I don't know how the npm publishing process works then...

@gabrielschulhof
Copy link

@Wind-rider, I'm not sure that having a symbolic link (such as js/node_modules/iotivity-node/index.d.ts) is portable across platforms. Have you tested the TypeScript examples on Windows?

@hansmbakker
Copy link
Contributor Author

@gabrielschulhof you're right, Windows doesn't understand the symbolic link. It was only in there to make the examples work, but I see I moved the ts examples to a separate folder so it's not needed there anymore.

As for whether the examples work on windows - they are not platform-dependent; Typescript itself is not platform dependent. I'm having installation issues with iotivity-node on Windows (line ending issue probably; annoying...) so I couldn't test running them.

There is are two issues left in the high-level-resource-creation-server code - Promise.then(...) seems to expect a promise-like parameter but there are () => void style functions used on lines 43 and 50, which makes the compiler complain.

@hansmbakker
Copy link
Contributor Author

Is there any feedback or thoughts on this?

@gabrielschulhof
Copy link

@Wind-rider just so you know, I'm still considering merging this PR, however, it may need to be updated because we're likely going to do a revamp of the high-level API to reflect the changes in the spec.

It would also be nice if you could add a test to make sure that the declarations are in sync with the actual API. That way you'd force a build failure every time we update the API, and we'd remember to also change the declarations ;)

@gabrielschulhof
Copy link

@Wind-rider I guess, depending on the timing of the implementation of the API change, this PR could land first, and then the API change would also involve the .ts files.

@hansmbakker
Copy link
Contributor Author

hansmbakker commented Aug 30, 2017

it also depends on the choice of whether to have iotivity-node written in typescript that compiles to js + typings, or whether to have it as js + manual typings.

Technically, having it written in typescript would be the most elegant solution (no need to keep typings in sync, cleaner code in the library)

@hansmbakker
Copy link
Contributor Author

@gabrielschulhof quite some time has passed, how is the new api coming along? Have you been able to take a look into TypeScript?

@gabrielschulhof
Copy link

@Wind-rider got distracted with security and API revamps, and other projects. I'll take another look ASAP. Thanks for sticking with it!

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

Successfully merging this pull request may close these issues.

4 participants