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

[discussion] Adding automatic Noble support via bindings #25

Open
gilesvangruisen opened this issue Dec 6, 2016 · 4 comments
Open

[discussion] Adding automatic Noble support via bindings #25

gilesvangruisen opened this issue Dec 6, 2016 · 4 comments

Comments

@gilesvangruisen
Copy link
Contributor

gilesvangruisen commented Dec 6, 2016

Hey Jacob!!

Thanks so much for putting in the work to get Noble running within React Native. I'm super impressed by your work here implementing the bindings in both Obj-C and Java, as well as your React Native PR adding support for deep shimming.

I was thinking about the issues around deep shimming and poking the Noble lib itself, and I think we might potentially be able to get around the need for deep shimming and also be able to import noble, which would rely on this when running in RN, rather than importing this library directly.

I think we can potentially solve this by adding direct binding support and including react-native-ble for those bindings. This could potentially get around the deep shimming issue if react-native-ble is included in noble's dependencies. However, that's obviously that's not ideal for every other project using noble, so I think we'd instead want to include react-native-ble under the "react-native" field in noble's package.json.

I'm going to take a stab at this locally and see what I can get to work. I'll post an update here with what I find.

Also: looks like that React Native PR #9899 was closed. If you're too busy to finish that up, I'm happy to take it over to make those changes and credit you.

Thanks again for putting this library together!!

@jacobrosenthal
Copy link
Owner

Hi
Love to see what you can do. Im fairly down on this project at the moment. Without tooling fixes it just doesnt have value.

Of note I found these guys' bindings that look nice with tests and all. I could see wrapping their stuff with the noble api instead of what I have here.

@techniq
Copy link

techniq commented Dec 7, 2016

Hi @jacobrosenthal and @gilesvangruisen. I've been attempting to use ble-serial with react-native-ble to shim it's use of noble but I've been running into various issues, which I think might be due to the deep shimming mentioned above.

Using require('noble') within index.android.js works fine but require('noble') used within ble-serial gives the RSOD with Unknown named module: 'tls'

I was just looking for confirmation that this is due to the deep shimming issue mentioned. Thanks.

@gilesvangruisen
Copy link
Contributor Author

Hey @techniq! That sounds like what I'm running into. I'd like to write a library for a specific BLE-enabled device that relies on Noble and is able to be used (though not exclusively) within a React Native project.

Currently, the biggest issues I'm running into are React Native's lack of deep-shimming, and React Native's lack of infrastructure for automatically swapping builtin node modules (like 'os', 'util', 'crypto', etc.) with React Native-supported equivalents.

That said, I have managed to successfully require('noble') directly in a project. I'm running a fork of RN 0.39 that adds support for deep-shimming (based on @jacobrosenthal's proposed changes). This lets me run a fork of Noble that adds bindings for React Native via a fork of this library that exposes those bindings directly. It also currently relies on a fork of react-native-os which exposes more useful information using the RN Platform module.

The deep-shimming PR is what lets us swap statements requiring 'os' to instead require 'react-native-os' by specifying the following in my package.json:

{
  …
  "react-native": {
    "os": "react-native-os"
  }
}

If you take a look at my fork of Noble, you may notice some commented lines. Unfortunately these require statements still fail, and dynamic requires are not possible with the React Native packager.

The best way around this I see currently is unfortunately to either split each of these bindings into their own repos and selectively exclude them depending on the platform that we're currently building for (by specifying "binding-module-name": false in our package.json. The other option is to shim those requires with an empty module, so that the require passes. (In RN, we don't care what's in there, because no calls will be made, we only care that the require statement doesn't throw an error).

Ultimately, I'd like to add support to the React Native packager to automatically swap node builtins for React Native-supported equivalents by maintaining a list/mapping of those modules. I think will require a bit of RN discussion first, so I'm hoping to get an initial version of that working this week so we can at least get the ball rolling.

I'd love to hear your thoughts on my ways around this, and if I'm missing anything glaringly obvious. I don't think adding automatic module swapping to RN will be that difficult. Most of the pieces already exists, I think it's simply a matter of putting them together and getting it merged.

@techniq
Copy link

techniq commented Dec 11, 2016

Hey @gilesvangruisen! I agree with everything you said 👍 .

While I work a great deal with React, I haven't used React Native extensively to add much to the discussion. I do wish React Native had "official" support for Node's builtins libraries via dynamic shimming / etc, and packager supported dynamic imports like webpack's require.ensure (or the new import() that webpack/others are adding support for).

Since they provide polyfills for browser APIs such as fetch, navigator.geolocation, and css/flexbox, it seems logical to add this at the "framework" level for the basic libraries alot of packages on npm require. I know my limited use of rn-nodeify has been fraught with problems, but understand it's the best way to work around some of these issues until something more official is available.

It seems like the PR by @jacobrosenthal had a lot of promise, but the RN devs were initially slow to respond, but hopefully giving it another go like you're suggesting will get it refined and merged. Let me know when you have a PR submitted and I'll keep an eye on it.

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

No branches or pull requests

3 participants