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

Multiple big improvements #31

Merged
merged 13 commits into from May 12, 2022
Merged

Multiple big improvements #31

merged 13 commits into from May 12, 2022

Conversation

Joolee
Copy link
Contributor

@Joolee Joolee commented Mar 15, 2022

Changes

  • Fixed Wake on Lan feature (Allows to turn the TV on when it has been off for >10 minutes)
  • Made app ~75% smaller by removing or substituting big dependencies (Makes development faster, reduces memory (by ~33%) and CPU usage and greatly reduces possible vulnerabilities)
  • Added English/Dutch translations for remote button names (in the action card)
  • Stop polling the TV when you remove it from Homey (Might fix some crashes)
  • Split notifyChange and device polling into separate threads (should stop on/off flickering)

@lucasvdh
Copy link
Owner

Hi @Joolee,

Thank you for this big contribution! 🙌

I've not been able to really give this app as much love as I would've liked because I'm still struggling with RSI. I will have a look at this pull request as soon as I'm back from vacation (in about a week).

@Joolee
Copy link
Contributor Author

Joolee commented Mar 17, 2022

Enjoy your vacation :)

Copy link
Owner

@lucasvdh lucasvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Joolee overall this is looking very good, but I'd like to clarify my comment below.

Was this a mistake or an intentional change for a reason that I'm missing?

@@ -3,13 +3,8 @@
"name": "philips-jointspace",
"version": "0.5.1",
"dependencies": {
"crypto": "^1.0.1",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The crypto dependency is still used during pairing but no longer referenced in the package.json.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Homey's flavour of nodejs is compiled with support for the crypto core module, installing a third-party library from npm should not be necessary.
Though, reading the documentation, it would be probably be better to require the module with the 'node:' prefix
https://nodejs.org/api/modules.html#core-modules
https://nodejs.org/api/crypto.html#crypto

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clearing things up!

@lucasvdh lucasvdh merged commit 9cccd94 into lucasvdh:beta May 12, 2022
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.

None yet

2 participants