Migrating project to Typescript #2476

Supereg · 2020-04-04T14:40:42Z

Okay, so. I think that's the third pull request regarding a typescript migration on this project (#2266, #2393). Both of them a currently not able to get merged and both of them don't seem to work (at least that is what I've been told).
[Edit: had a look at them, and there are some not so got parts about them]

This PR aims to finally migrate this project. And I think with the ongoing beta phase we have a higher confidence to release it to the public (without backtracking on prior testing and code review!).

Currently this PR only contains configuration changes for the new project. The idea was to split configuration changes and actual code changes into (at least) two commits, to improve code review. As I don't know what exact project configuration is preferred, I wanted to use this as a starting point to propose some configurations and let them be discussed here. Any requested changes can currently be incorporated into the now ongoing process of migration.
Ideally this PR should suggest to other contributors to maybe delay some of their PRs, if they plan to make bigger changes in the Code, as otherwise this will result in a big ass merge conflict 🙃

The plan is to squash all commits when the PR is ready, as I currently do not guarantee a working project on every commit

Changes

Project Configuration

Migrated to Typescript
- created tsconfig.json
- created src/ and moved contents of lib/ into it (lib/ will be the output dir so backwards compatibility for project structure is provided)
Added ESLint
Added jest for future unit tests
Adjusted GitHub Actions "build" jobs

Current PR Status: READY TO MERGE

Supereg · 2020-04-04T15:57:10Z

Okay, so to further improve the process of code review I try to create a commit for every migrated file. Don't know how well this is gonna be for the bigger ones, but works for all the util files.

The current idea for merging would be, that alle commits get squashed into two commits. The first one contains all configuration and renaming of files. And the second one the actual changes inside the source code files. This way we can preserve git blame history.

ebaauw

I don't have any experience with TypeScript yet. I'm a bit worried about the increased footprint, but I understand run-time, you'd only use the JavaScript files generated by the TypeScript compiler. I'm happy to test my plugins against a beta, but I'm afraid I won't be of much value in a code review.

I haven't yet decided whether to migrate my own plugins to TypeScript. I'm a big fan of strong typing, and hate JavaScript for not supporting it. I'm currently using standard as style (no semi-columns); that doesn't seem compatible with TypeScript. Still busy migrating to the dynamic platform plugin model, I think I want to finish that before picking up another big migration.

Supereg · 2020-04-04T16:58:51Z

I don't have any experience with TypeScript yet. I'm a bit worried about the increased footprint, but I understand run-time, you'd only use the JavaScript files generated by the TypeScript compiler. I'm happy to test my plugins against a beta, but I'm afraid I won't be of much value in a code review.

I'm happy to help if you got any questions. Nonetheless it could be helpful if you try to check changes made, to maybe spot any errors made in logic (not necessarily in language).
Typescript transpiles into JavaScript, exactly. This happens before we publish the package. So we still deploy JavaScript (and additional type files generated by the Typescript compiler used by IDEs). As a result any normal Javascript homebridge plugin can still be used.

But you could also run Typescript directly using ts-node and basically compile it on the fly, which results in a reduced execution/startup time though.

ebaauw · 2020-04-04T17:38:03Z

What IDE do you use or recommend? I currently use Atom with the standard plugin for some code checking, but not nearly enough.

Supereg · 2020-04-04T18:05:13Z

What IDE do you use or recommend? I currently use Atom with the standard plugin for some code checking, but not nearly enough.

I'm currently using WebStorm. Has some pretty nice auto completion and some decent default linting, but also adopts dynamically if project specifies an eslint configuration for example. I know that VSCode does the same AFAIK.

hassankhan · 2020-04-04T20:35:00Z

Just wanted to thank you @Supereg 👍

Supereg · 2020-04-05T19:44:44Z

BridgeSetupManager was hell to type 🙃

Supereg · 2020-04-07T00:36:24Z

Last thing to migrate is server.ts

src/plugin.ts

Supereg · 2020-04-07T13:19:55Z

Some quick question, If it's okay I would move some of the stuff inside server.ts to places they suite better. For example some of the plugin loading stuff inside server.ts would get move into plugin.ts. Don't know what's better for review. Otherwise I would just do a migration while trying to keep most of the current structure and do all the refactoring in another PR or in a follow up commit in this PR. @oznu

oznu · 2020-04-07T21:55:17Z

It might be easier to keep the structure similar to the current code for now. Maybe we can do an initial beta rerelease with these changes (once complete) then a follow up beta with the restructure - all before 1.0.0?

Supereg · 2020-04-07T22:03:48Z

Okay great. Thought readability could definitely be improved just my moving some config loading and plugin loading into the respective plugin.ts and user.ts (which is basically a config.ts).

Supereg · 2020-04-08T03:25:37Z

Okay migration should be complete. I tested it on a basic installation with homebridge-dummy enabled. Would want to run/test it on my local instance before merging it. You all are welcome to do your own tests though.

oznu · 2020-04-08T03:29:21Z

Nice work! Thanks @Supereg.

I'll test this out tonight.

oznu · 2020-04-08T07:29:02Z

Should we include https://github.com/evanw/node-source-map-support ?

This will make stack traces alot easier to follow. eg.

[4/8/2020, 17:54:17] Error loading platform requested in your config.json at position 2
[4/8/2020, 17:54:17] Error: The requested platform 'esp-irrigation-controller' was not registered by any plugin.
    at HomebridgeAPI.platform (/usr/local/lib/node_modules/homebridge/src/api.ts:258:23)

Note it provides the typescript line number in the stack trace.

I just added this to the top of cli.ts:

import 'source-map-support/register'

oznu · 2020-04-08T07:42:07Z

f6f3d87 needs to go in to prevent these errors:

[4/8/2020, 5:39:36 PM] TypeError: Cannot read property 'updateValue' of undefined
    at /usr/local/lib/node_modules/homebridge/src/server.ts:523:82

oznu · 2020-04-08T07:45:26Z

scoped module paths are not resolved correctly.

Example: https://www.npmjs.com/package/@kacepe/homebridge-shelly

Homebridge tried to load the plugin from /homebridge/node_modules/homebridge-shelly instead of /homebridge/node_modules/@kacepe/homebridge-shelly/

[4/8/2020, 5:43:33 PM] Plugin /homebridge/node_modules/homebridge-shelly does not contain a package.json.

The layout of scoped npm modules on disk looks like this:

node_modules
    homebridge
    homebridge-config-ui-x
    homebridge-hue
    @oznu 
        homebridge-scoped-plugin
        hap-client
    @kacepe
        homebridge-shelly

oznu · 2020-04-08T08:08:31Z

.eslintrc

+  ],
+  "rules": {
+    "quotes": ["error", "double"],
+    "indent": ["error", 4, { "SwitchCase": 1 }],


HAPNodeJS uses two, not sure if we want to follow in style.

(Disclaimer: I like two 😬)

That is exactly one of the things I wanted to be discussed with my initial commit.

HAP-NodeJS is basically using both 😅 It's not quite consistent.
I personally grew up using four so that's where the default values is coming from. But idk, as long we can be consistent on one.

Supereg · 2020-04-08T14:26:26Z

f6f3d87 needs to go in to prevent these errors:

[4/8/2020, 5:39:36 PM] TypeError: Cannot read property 'updateValue' of undefined
    at /usr/local/lib/node_modules/homebridge/src/server.ts:523:82

Oh that's where this part of the code is coming from I was surprisingly unfamiliar with. You may made the error go away, but HAP-NodeJSs getCharacteristic is a bit odd in the way that it will forcefully add the characteristic anyway (not just if it is contained in the optionalCharacteristics). I rather think my original PR was passing the wrong type. Your fix makes the error go away but also prevents the characteristics ever get copied.
This PR fixed it my passing the typeof characteristic. Works and I do not have any issues with it. Or if the issue persist could you just state which characteristics are exposed in your AccessoryInformationService?

Supereg · 2020-04-08T14:59:30Z

Added two commits to add source-map support and fixing scoped plugins. @oznu

Supereg · 2020-04-08T16:23:10Z

f6f3d87 needs to go in to prevent these errors:

[4/8/2020, 5:39:36 PM] TypeError: Cannot read property 'updateValue' of undefined
    at /usr/local/lib/node_modules/homebridge/src/server.ts:523:82

Okay this is actually somewhat of a bigger problem. The getCharacteristic of HAP-NodeJS is simply not designed to do that. I created a method over at https://github.com/KhaosT/HAP-NodeJS/pull/807 to create a HAP-NodeJS supported way of replacing a services characteristics with characteristics from another service. This also deals with get and set listeners thus fully replacing #2169.

Supereg · 2020-04-08T22:21:53Z

Okay, so the current state of the PR is now feature complete. It runs already the latest HAP-NodeJS beta (with the revamped Camera controllers and stuff, but support for this will be moved to another PR). Currently runs smooth for me on my personal instance (with some cameras and some basic plugins).
I'm currently a bit unhappy about the way plugin devs will interface with hap-nodejs exported classes. Currently everything is exported from hap-nodejs through homebridge. But IDEs (at least for my experience) have a hard time auto completing those imports which come from hap-nodejs (as they seem to not recursively dig through all modules). Anyone an idea how to improve on that? (@oznu). I will still do some experimenting and create some example plugins (for myself), to check the usage of the API.

Besides that I'm just waiting for everyone doing a code review to finish with that. Testers are welcome too. If I get the okay, I will restructure this commit as above explained to only consist of two commits. Only after that I would suggest to merge into the beta branch.

oznu · 2020-04-09T13:23:46Z

Thanks @Supereg, the new update are working well.

I'm happy for this to be merged into the beta branch when you are.

Supereg · 2020-04-09T16:26:59Z

Okay, squashed all changes into two commits (which should be preserved when merging).
Reformatted code to enforce indent of 2.

I'm ready to merge (into beta!). I would address any further changes to the exposure of the types for plugin developers in a follow up PR if that's okay.
@oznu

donavanbecker requested a review from a team April 4, 2020 15:50

Supereg changed the title ~~Setup typescript project, eslint and prettier~~ Migrating project to Typescript Apr 4, 2020

donavanbecker added the enhancement label Apr 4, 2020

ebaauw reviewed Apr 4, 2020

View reviewed changes

Supereg force-pushed the typescript branch from 8029de3 to 105c29d Compare April 4, 2020 17:36

Supereg force-pushed the typescript branch 2 times, most recently from e6f6d00 to 6554897 Compare April 4, 2020 18:01

This was referenced Apr 4, 2020

Migration to TS #2393

Closed

Setup ESLint and Jest #2358

Closed

Convert project to TypeScript #2266

Closed

Supereg force-pushed the typescript branch 5 times, most recently from 615b1c4 to 351542c Compare April 6, 2020 23:18

oznu reviewed Apr 7, 2020

View reviewed changes

src/plugin.ts Outdated Show resolved Hide resolved

Supereg force-pushed the typescript branch 3 times, most recently from 62d1cbb to 9462a55 Compare April 7, 2020 18:13

Supereg force-pushed the typescript branch from 9462a55 to 2d7de7f Compare April 8, 2020 03:22

Supereg force-pushed the typescript branch 2 times, most recently from 53d4f09 to 578a178 Compare April 8, 2020 03:44

oznu reviewed Apr 8, 2020

View reviewed changes

Supereg force-pushed the typescript branch from 8f54672 to b71b705 Compare April 8, 2020 14:51

donavanbecker mentioned this pull request Apr 8, 2020

Remember listeners on AccessoryInformation if returned by getServices #2169

Closed

Supereg force-pushed the typescript branch 2 times, most recently from c4b3274 to 0e9ff3a Compare April 8, 2020 22:10

Supereg force-pushed the typescript branch from 0e9ff3a to a1b1fe1 Compare April 9, 2020 14:36

Setup typescript project, eslint and prettier

502ce30

Supereg force-pushed the typescript branch from a1b1fe1 to 0dc42f7 Compare April 9, 2020 14:40

Migrate project to Typescript

b324684

Supereg force-pushed the typescript branch from 0dc42f7 to b324684 Compare April 9, 2020 20:42

Supereg merged commit 04c5889 into homebridge:beta Apr 9, 2020

Supereg deleted the typescript branch April 15, 2020 02:58

Supereg mentioned this pull request Apr 20, 2020

Contribution #2359

Closed

oznu mentioned this pull request Apr 27, 2020

v1.0.0 #2516

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrating project to Typescript #2476

Migrating project to Typescript #2476

Supereg commented Apr 4, 2020 •

edited

Loading

Supereg commented Apr 4, 2020 •

edited

Loading

ebaauw left a comment

Supereg commented Apr 4, 2020

ebaauw commented Apr 4, 2020 •

edited

Loading

Supereg commented Apr 4, 2020

hassankhan commented Apr 4, 2020

Supereg commented Apr 5, 2020

Supereg commented Apr 7, 2020

Supereg commented Apr 7, 2020

oznu commented Apr 7, 2020

Supereg commented Apr 7, 2020

Supereg commented Apr 8, 2020

oznu commented Apr 8, 2020

oznu commented Apr 8, 2020 •

edited

Loading

oznu commented Apr 8, 2020

oznu commented Apr 8, 2020 •

edited

Loading

oznu Apr 8, 2020 •

edited

Loading

Supereg Apr 8, 2020 •

edited

Loading

Supereg commented Apr 8, 2020

Supereg commented Apr 8, 2020

Supereg commented Apr 8, 2020

Supereg commented Apr 8, 2020

oznu commented Apr 9, 2020

Supereg commented Apr 9, 2020

Migrating project to Typescript #2476

Migrating project to Typescript #2476

Conversation

Supereg commented Apr 4, 2020 • edited Loading

Changes

Project Configuration

Supereg commented Apr 4, 2020 • edited Loading

ebaauw left a comment

Choose a reason for hiding this comment

Supereg commented Apr 4, 2020

ebaauw commented Apr 4, 2020 • edited Loading

Supereg commented Apr 4, 2020

hassankhan commented Apr 4, 2020

Supereg commented Apr 5, 2020

Supereg commented Apr 7, 2020

Supereg commented Apr 7, 2020

oznu commented Apr 7, 2020

Supereg commented Apr 7, 2020

Supereg commented Apr 8, 2020

oznu commented Apr 8, 2020

oznu commented Apr 8, 2020 • edited Loading

oznu commented Apr 8, 2020

oznu commented Apr 8, 2020 • edited Loading

oznu Apr 8, 2020 • edited Loading

Choose a reason for hiding this comment

Supereg Apr 8, 2020 • edited Loading

Choose a reason for hiding this comment

Supereg commented Apr 8, 2020

Supereg commented Apr 8, 2020

Supereg commented Apr 8, 2020

Supereg commented Apr 8, 2020

oznu commented Apr 9, 2020

Supereg commented Apr 9, 2020

Supereg commented Apr 4, 2020 •

edited

Loading

Supereg commented Apr 4, 2020 •

edited

Loading

ebaauw commented Apr 4, 2020 •

edited

Loading

oznu commented Apr 8, 2020 •

edited

Loading

oznu commented Apr 8, 2020 •

edited

Loading

oznu Apr 8, 2020 •

edited

Loading

Supereg Apr 8, 2020 •

edited

Loading