Change to the extracted NetInfo package

[sbeca]

This is a follow-up PR to what I started in PR #1059.

The goal of this PR is to change to the extracted NetInfo package instead of version bundled with React Native. React Native deprecated the bundled version in release v0.59.0 as part of their Lean Core initiative.

This is still a work-in-progress because this PR doesn't yet support Windows. react-native-netinfo doesn't include Windows support and has changed enough from the code that used to be a part of React Native core that it doesn't work with the react-native-windows codebase. I've started work on porting and updating the NetInfo code from react-native-windows to react-native-netinfo but I'm not sure how long that will take. My progress can be tracked here: https://github.com/sbeca/react-native-netinfo/tree/windows-support

I wanted to get this PR out in the public before it was ready in case someone else was also working on the issue. I've noticed that @mikehardy posted the following issue very recently so if we're doubling up on work, it would be good to know: react-native-netinfo/react-native-netinfo#100

[mikehardy]

@sbeca I don't think we are doubling up - I'm not doing any heavy-lifting at all, I'm more on the end-user level just trying to test other's work to verify, and smoothing rough edges while you're making the things really work - which I appreciate a lot. @matt-oakes might be interested in this though, I think of him as the goto for netinfo

[sbeca]

FYI, I have just submitted my PR to the NetInfo repo to add Windows support: react-native-netinfo/react-native-netinfo#103

If/when that PR is accepted, I'll update this PR to point to the new version and then this PR should be good to go, pending the decision to keep it in ReactXP core or move it into its own module.

[matt-oakes]

The PR to add Windows support to the NetInfo module is ongoing. We'll also need to add web support for you to use it correctly with ReactXP. There is an open PR for this, but it's pretty out of date. I can try to update it soon, unless someone else wants to pick it up.

I've not used ReactXP before, but if we did add Windows and Web support to the NetInfo module, is there any need for this to be in ReactXP? The plugin code seems to just translate the NetInfo API to a different format. Would it not be better for developers to use the NetInfo module directly and they would automatically get new features when they are released? Am I missing something about how ReactXP works that means this is not possible?

[deregtd]

In the past, there were several bugs and API quirks with netinfo that we abstracted away between platforms in ReactXP. I assume some of those still exist to this day, since it's never been a Facebook goal to have uniform behavior across platforms, but I haven't looked at changes recently.

[matt-oakes]

@deregtd Thanks for the reply! Can you recall what the issues were? I'm now maintaining the module outside of Facebook.

[deregtd]

@matt-oakes I think some of the original nuances have been lost at this point. I'm recalling stuff from the early days where the state wasn't available until a certain amount of time had passed, so we had to check it later on, and provide some buffering in between. Looking at the code now, it's more straightforward, though we still shim between the older fetch and the newer promise calls to ensure compat between RN versions:

https://github.com/microsoft/reactxp/blob/master/src/native-common/Network.ts

I think it appears that the newer implementation is pretty good, cross-plat, without any of the nuance we were working around originally.

The other main thing that ReactXP brings to the table is integration with web. So there's also the simple web abstraction of the same interface:

https://github.com/microsoft/reactxp/blob/master/src/web/Network.ts

If you integrate that ALL, including web, into the NetInfo package, then I don't see why it'd need to be in ReactXP at all anymore. Just make sure that the web support doesn't depend on react-native-web in any way.

[matt-oakes]

@deregtd Thanks for the reply! I agree that web support needs to be in before it can be used with ReactXP. As mentioned before, there is an open PR for it here, but it needs some work now as the code was update pretty substantially since it was opened. It mentions react-native-web in the PR notes, but the code doesn't seem specific to that at all. If no one else picks it up, then I can take a look at adding it in. Just to note that I don't need web support so it will be a little way down my TODO list.

[sbeca]

FYI, my PR to react-native-netinfo to add Windows support has now been merged and a new version released on npm so I have just updated this PR to point to it. This means that this PR is now ready for final review.

Note, if you want to test the Windows build of the RXPTest sample, I've found using the following local changes to samples/RXPTest/package.json gives the best compatibility results:

-    "react": "16.8.3",
-    "react-dom": "16.8.3",
-    "react-native": "0.59.5",
+    "react": "16.6.3",
+    "react-dom": "16.6.3",
+    "react-native": "0.57.8",

[matt-oakes]

Just to note, the NetInfo module still doesn't provide support for web. Is this an issue for you with this being merged in now?

[deregtd]

Oh, yes, that's bad. ReactXP needs to maintain support for all platforms and not regress functionality.

[matt-oakes]

Sorry. I mentioned it above but should have been clearer. Do you want to revert this until web support is in NetInfo module?

[erictraut]

Yes, I'll revert it.

[erictraut]

OK, it's reverted. Sorry for jumping the gun on that.

[berickson1]

To be fair, the current state of the extracted module is the same as reactxp core was, there are some gaps for NetInfo package on web.
I don't think the intention of this PR was to add functionality, but to move the package out of the core. I agree that web support should be implemented, but it might be better suited as a separate changeset

[sbeca]

Yeah, this PR was fine to merge as it was because web support was still being handled by ReactXP specific code and not relying on non-existent web support in the react-native-netinfo library.

Does ReactXP want to maintain it's own netinfo library or does it want to eventually remove all NetInfo support and just recommend using the react-native-netinfo library?