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

[v5] Cusomisation options & removal of deprecated methods #230

Merged
merged 10 commits into from
Dec 8, 2019

Conversation

matt-oakes
Copy link
Collaborator

@matt-oakes matt-oakes commented Oct 6, 2019

Overview

This PR is for the future 5.0.0 release. The main changes are:

  • Removal of the methods which has been deprecated and undocumented since 2.0.0. These were the original method names from when this library was part of the React Native core.
  • Allow the internet reachability test to be customised to allow this library to work in all locations.

Breaking changes:

  • Deprecated methods have been removed.
  • The library will not begin monitoring the internet connectivity state until the first time you call a method. This may change the results you get when calling fetch().

How to test this

yarn add @react-native-community/netinfo@next

or

npm install --save @react-native-community/netinfo@next

Please report back if this PR is working well for you. If you have any issues, open a new issue with [v5] at the start of the issue title.

Fixes #201

Fixes #147

README.md Outdated
```javascript
NetInfo.configure({
reachabilityUrl: 'https://clients3.google.com/generate_204',
reachabilityTest: response => response.status === 204,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be an async/await function
reachabilityTest: async response => await response.status === 204,
because TS is complaining "Type 'boolean' is not assignable to type 'Promise'.ts(2322)"

@mwmcode
Copy link

mwmcode commented Oct 7, 2019

@matt-oakes I just tried changing the useNetInfo hook to accept user config and it seems to work.

export function useNetInfo(
  userConfig?: Partial<Types.NetInfoConfiguration>,
): Types.NetInfoState {
  if (userConfig) {
    configure(userConfig);
  }
  // ... same code
  const [netInfo, setNetInfo] = useState<Types.NetInfoState>({
    type: Types.NetInfoStateType.unknown,
    isConnected: false,
    isInternetReachable: false,
    details: null,
  });

  useEffect((): (() => void) => {
    return addEventListener(setNetInfo);
  }, []);

  return netInfo;
}

In my app

export default function OtherScreen() {
  const { isConnected, type } = useNetInfo({
    reachabilityUrl: 'http://detectportal.firefox.com',
    reachabilityTest: async response => (await response.text()) === 'success',
  });
....
}
  • Not sure if this is the right place for this comment, if it is not please advise
  • If user finds it "ugly" to pass an object to the hook, they can always create a custom one
  • They way I "know" it works, is because my firewall app is showing a warning that the app is trying to reach http://detectportal.firefox.com instead of google one
  • Thoughts?

@MrLoh
Copy link

MrLoh commented Nov 15, 2019

this looks really good, any idea on when this might be released?

@matt-oakes
Copy link
Collaborator Author

I still need to do some work on this before I can release it. See the list above for details.

README.md Outdated
NetInfo.configure({
reachabilityUrl: 'https://clients3.google.com/generate_204',
reachabilityTest: response => response.status === 204,
reachabilityShortTimeout: 60 * 1000, // 60s
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
reachabilityShortTimeout: 60 * 1000, // 60s
reachabilityShortTimeout: 5 * 1000, // 5s

README.md Outdated
reachabilityUrl: 'https://clients3.google.com/generate_204',
reachabilityTest: response => response.status === 204,
reachabilityShortTimeout: 60 * 1000, // 60s
reachabilityLongTimeout: 5 * 1000, // 5s
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
reachabilityLongTimeout: 5 * 1000, // 5s
reachabilityLongTimeout: 60 * 1000, // 60s

@matt-oakes matt-oakes marked this pull request as ready for review December 8, 2019 09:32
@matt-oakes matt-oakes merged commit fab577d into master Dec 8, 2019
react-native-community-bot pushed a commit that referenced this pull request Dec 8, 2019
# [5.0.0](v4.7.0...v5.0.0) (2019-12-08)

### Features

* Configuration & removal of deprecated methods ([#230](#230)) ([fab577d](fab577d))

### BREAKING CHANGES

* Previously deprecated methods have not been removed. These methods have been deprecated since this library was extracted from the core of React Native. Most users will not have any issues with migrating if they were not ignoring the previous warnings.

Added a new way to configure the reachability URL that the library uses on iOS to check for an internet connection. The default is still to use the Google Chrome URL, however, you can now customise this URL, test function, and the timeouts that are used.
@react-native-community-bot
Copy link
Collaborator

🎉 This PR is included in version 5.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

isInternetReachable should be initialized to undefined rather than null Reachability [China🇨🇳]
5 participants