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

Update README.md #588

Merged
merged 1 commit into from
Apr 21, 2022
Merged

Update README.md #588

merged 1 commit into from
Apr 21, 2022

Conversation

JDMathew
Copy link
Contributor

Overview

Adding optional chaining as isConnected may be null causing an error:

Uncaught TypeError: Cannot read properties of null (reading 'toString')

Adding optional chaining as isConnected may be null causing an error:
>Uncaught TypeError: Cannot read properties of null (reading 'toString')
Copy link
Contributor

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

I can't accept this, isConnected is not nullable:

isConnected: true;

If there's a case where it's null, some portion of the code has an error, and if you can reproduce that, it's really important to fix the root cause, which is that something is generating a NetInfoConnectedState without the required parameters

@JDMathew
Copy link
Contributor Author

JDMathew commented Mar 30, 2022

Thanks @mikehardy, I think there might be an underlying issue but I haven't tested it in a fresh RN project. According to the docs isConnected is nullable - see here.

For context, I've written a custom hook which I call on initialisation of my app. It makes use of the useNetInfo hook. Initially the state returned from useNetInfo is an object with all keys being null, it then updates with the correct state.

i.e initially this is returned:

{type: "unknown", isConnected: null, isInternetReachable: null, details: null}

There does also seem to be multiple re-renders coming back from the useNetInfo hook as various values change and so I am using a ref in my custom hook to prevent showing the netInfo multiple times to the user.

Here is the custom hook I wrote:

function useNetInfoStatus() {
  const previouslyOfflineRef = useRef(false)
  const netInfoState = useNetInfo()

  useEffect(() => {
    const { type, details } = netInfoState
    const online = netInfoState.isConnected && netInfoState.isInternetReachable

    if (online === false && !previouslyOfflineRef.current) {
      Snackbar.show({
        text: 'Your device is currently offline',
        duration: Snackbar.LENGTH_SHORT,
      })
      previouslyOfflineRef.current = true
    }

    if (online && previouslyOfflineRef.current) {
      Snackbar.show({
        text: 'Back online',
        duration: Snackbar.LENGTH_SHORT,
      })
      previouslyOfflineRef.current = false
    }

    if (type === NetInfoStateType.wifi && details.strength && details.strength < 3) {
      Snackbar.show({
        text: `Your internet connection is weak \n${glossary.organization} may not function properly`,
        duration: Snackbar.LENGTH_LONG,
      })
    }
  }, [netInfoState])
}

export default useNetInfoStatus

@JDMathew
Copy link
Contributor Author

JDMathew commented Mar 30, 2022

@mikehardy I think this is intended behaviour, digging into the NetInfo source code you can see that the hook is initialised with isConnected as null hence the initially returned value of null. See here:

isConnected: null,

Commit responsible: 4d84f14

BREAKING CHANGE: When the connection state is unknown, the isConnected and isInternetConnected properties are now set to null rather than false. This allow you to easily detect the initial "unknown" state before the state is detected and set to a boolean.

@JDMathew
Copy link
Contributor Author

JDMathew commented Mar 30, 2022

I can't accept this, isConnected is not nullable:

isConnected: true;

If there's a case where it's null, some portion of the code has an error, and if you can reproduce that, it's really important to fix the root cause, which is that something is generating a NetInfoConnectedState without the required parameters

@mikehardy, The type you linked is not the returned type for useNetInfo()

but rather:

export type NetInfoState = NetInfoDisconnectedStates | NetInfoConnectedStates;

This type is in fact correct, i.e no underlying issue, and isConnected results in boolean | null.

I see now that I wasn't actually getting a warning in my console for the null case however, as null and undefined are subtypes of all other types and I had not enabled strictNullChecks ("strictNullChecks": true). This meant that boolean | null "narrowed" to boolean and I had no warning.

This depends on your typescript version

  • before 2.0 null can be returned on ("is in the domain of") any type, so boolean will be the isConnected type
  • starting with 2.0, if you enable --strictNullChecks then you have to specify that a type can return null. So isConnected type will be boolean | null

Details here

@mikehardy
Copy link
Contributor

Ahh - definitely need to queue this for review again, thanks for the extra information and taking the time to make a PR in the first place, much appreciated

@mikehardy mikehardy self-requested a review March 30, 2022 15:40
Copy link
Contributor

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

One of my other repos exploded with activity and I've been swamped, sorry for the delay.
This looks great on re-review, and I really appreciate the digging / details and that you posted it. Cheers

@mikehardy mikehardy merged commit 7f040ce into react-native-netinfo:master Apr 21, 2022
@matt-oakes
Copy link
Collaborator

🎉 This PR is included in version 8.3.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.

None yet

3 participants