Page MenuHomePhabricator

[native] Default to 'natDevHostname' from expo config
ClosedPublic

Authored by bartek on Jan 14 2023, 8:58 AM.
Tags
None
Referenced Files
F3244021: D6268.diff
Thu, Nov 14, 10:28 AM
Unknown Object (File)
Fri, Nov 8, 10:06 AM
Unknown Object (File)
Thu, Nov 7, 11:37 PM
Unknown Object (File)
Sat, Nov 2, 2:20 AM
Unknown Object (File)
Sat, Oct 26, 5:30 AM
Unknown Object (File)
Thu, Oct 17, 6:37 AM
Unknown Object (File)
Thu, Oct 17, 6:37 AM
Unknown Object (File)
Thu, Oct 17, 6:35 AM
Subscribers

Details

Summary

Updated the dev-hostname.js to try gathering the natDevHostname from the Expo config first.
If this fails, then it falls back to the existing facts/network.json flow.

If none is present, the warning will be displayed

Depends on D6267

Test Plan
  1. Deleted facts/network.json
  2. Opened the app on physical device
  3. No related warning is displayed
  4. Redux devtools are working

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bartek held this revision as a draft.
native/utils/dev-hostname.js
41–42 ↗(On Diff #20964)

This warning logic is displayed as I described in the diff description, but I'm worried about one case that we might want to handle, but I'm not sure how:

When the detected IP somehow fails and network.json has wrong value (e.g. somebody left an old value there) - the warning won't be displayed, but the connection will be failing with no meaningful message.

bartek published this revision for review.Jan 14 2023, 9:27 AM
ashoat added inline comments.
native/utils/dev-hostname.js
41–42 ↗(On Diff #20964)

That's a good case to consider... but hopefully it's not common if the autodetect logic works well, and I'm also not sure of an easy solution to it

This revision is now accepted and ready to land.Jan 14 2023, 5:18 PM

We should also update docs to strip all references to network.json

native/utils/dev-hostname.js
41–42 ↗(On Diff #20964)

I thought about it more and I think we can leave it.
There's no good solution without displaying excessive warnings either way and this situation will be rather rare. Also, if we remove the network.json entirely, this warning flow can be simplified and its content could then suggest setting COMM_NAT_DEV_HOSTNAME instead (introduced in D6293).
Another possibility is to display some messages directly in app.config.js - they'll be visible only in the Metro console, at the very beginning.

native/utils/dev-hostname.js
41–42 ↗(On Diff #20964)

I like COMM_NAT_DEV_HOSTNAME better than network.json because it's less "sneaky" – you'll probably have a change in your Git working copy (in package.json) if you're overriding it, so you're less likely to have a "silent failure" you can't figure out how to resolve.

Another possibility is to display some messages directly in app.config.js - they'll be visible only in the Metro console, at the very beginning.

I like this idea! I think it would be good to log for two cases:

  • Where autodetect fails, we can then request the user sets COMM_NAT_DEV_HOSTNAME
  • If COMM_NAT_DEV_HOSTNAME is set and does not match autodetect, we can display a message eg. "IP address autodetect has been overriden by COMM_NAT_DEV_HOSTNAME"
native/utils/dev-hostname.js
41–42 ↗(On Diff #20964)

Agreed, these cases make sense to me!

I created https://linear.app/comm/issue/ENG-2743/rework-networkjson-warning to track this further