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

Added Node 10 compatibility #32

Merged
merged 9 commits into from
Jan 26, 2019
Merged

Added Node 10 compatibility #32

merged 9 commits into from
Jan 26, 2019

Conversation

GorkaMM
Copy link
Contributor

@GorkaMM GorkaMM commented Jan 19, 2019

While official Noble repo adds support for Node 10, there's a fork that solved the compatibly issue published as @abandonware/noble.

For this fork I just modified the package.json and scanner.js files.

I'm currently running it on a Raspberry Pi and it works like a charm.

@hannseman
Copy link
Owner

Hi! Thanks for the PR. Yes I'm aware of that fork. The reason why I haven't switched is that I wanted to see if the author was interested in also bringing in support for MacOS Mojave as that to is broken, see #24.

I've opened the PR abandonware/noble#4 but haven't gotten any response what in almost a month which made me wonder if that fork really is the way to go. But maybe it's better to actually get Node 10 support and then start thinking about MacOS Mojave support.

The test suite is broken, it is probably caused by proxyquire patching noble and not @abandonware/noble. Also please remove the version-bump. When bringing in Node 10 support I plan on removing support for any versions older than Node 8 and thus requiring a new major version but for now you can just leave it unchanged.

@hannseman
Copy link
Owner

I've also opened an issue on noble-mac to check if they are interested in depending on the @abandonware-fork. Timeular/noble-mac#23

This would enable us to get support for both Mojave and Node 10 on linux.

@hannseman
Copy link
Owner

Perfect! Could you also run prettier on the code? There's some errors https://travis-ci.org/hannseman/homebridge-mi-hygrothermograph/jobs/484840460#L697

@hannseman
Copy link
Owner

hannseman commented Jan 26, 2019

Sorry I messed up your PR. Could you please rebase. You can remove:

matrix:
  allow_failures:
    - node_js: 10
    - node_js: node

As this PR fixes that. You can also remove Node 7 from the node_js list as it is no longer supported or patched. For the package-lock.json conflict just accept the file from master and then build a new one.

@GorkaMM
Copy link
Contributor Author

GorkaMM commented Jan 26, 2019

Working on it now…

@hannseman
Copy link
Owner

Awesome work! Thank you very much. Lets merge this. I'll make a new release ASAP.

@hannseman hannseman merged commit 0ae1d27 into hannseman:master Jan 26, 2019
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.

2 participants