Page MenuHomePhabricator

[native] Remove network.json fallbacks
ClosedPublic

Authored by bartek on Mar 12 2023, 11:36 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 24, 10:46 AM
Unknown Object (File)
Sun, Nov 24, 8:29 AM
Unknown Object (File)
Sun, Nov 24, 7:53 AM
Unknown Object (File)
Sun, Nov 24, 6:07 AM
Unknown Object (File)
Tue, Nov 5, 10:16 PM
Unknown Object (File)
Tue, Nov 5, 10:16 PM
Unknown Object (File)
Tue, Nov 5, 10:16 PM
Unknown Object (File)
Tue, Nov 5, 10:16 PM
Subscribers

Details

Summary

Addresses ENG-2742. ENG-2743

Autodetected IP has been staying with us for some time without issues, time to finally remove network.json.

Steps made:

  • Removed native/facts/network.json references as described in ENG-2742. Deleting it from .gitignore will force people still having this file to remove it
  • Reworked the warning as described in ENG-2743.
Test Plan
  • Ran yarn dev and opened mobile app. Ensured that it keeps working as expected.
  • Manually replaced ip.v4.sync() with undefined and ensured that:
    • The warning is displayed once in Metro
    • The warning is displayed once in-app
  • When the COMM_NAT_DEV_HOSTNAME env variable is set, the above warnings disappear

Diff Detail

Repository
rCOMM Comm
Branch
barthap/delete-network-json
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

bartek held this revision as a draft.
bartek published this revision for review.Mar 13 2023, 12:12 AM
ashoat added inline comments.
native/utils/dev-hostname.js
21–23 ↗(On Diff #23663)

Why do we need both this notice as well as the native/app.config.js? Does one show up during compile time, and the other during app runtime?

This revision is now accepted and ready to land.Mar 13 2023, 9:36 AM
native/utils/dev-hostname.js
21–23 ↗(On Diff #23663)

One shows up in the console when Metro starts, this one appears during runtime when neither autodetection nor env var are set. I'd prefer to leave only the Metro one, but it is easy to miss (maybe it could be colored or sth, but I'd need to install chalk dependency in such case).

Got it, I think it's okay to have both. Up to you

I'm going to leave both for now.

This revision was automatically updated to reflect the committed changes.