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

Issue/remove default config #264

Merged
merged 3 commits into from
Dec 20, 2019
Merged

Issue/remove default config #264

merged 3 commits into from
Dec 20, 2019

Conversation

lonnylot
Copy link
Contributor

Overview

Fixes issue described in #262.

useNetInfo is broken on multiple calls (which happens when components re-render).

The configuration is defaulted to {} and configure is always called. The call to configure then creates a new State instance. The addEventListener is only called on the first time and the listener is not re-added on subsequent calls.

Overall I don't see how passing a configuration param is doable here as any attempt to add an event listener results in a re-render. I have changed it to not have a default value so it can work as it did before. If you do pass in a configuration param then you'll need to add a listener separately, I guess?

Test Plan

I couldn't run the test suite as I got the following error

> @react-native-community/netinfo@5.0.0 test:jest /Users/lonnykapelushnik/Development/react-native-netinfo
> jest "/src/"

 FAIL  src/__tests__/fetch.ts
  ● Test suite failed to run

    TypeScript diagnostics (customize using `[jest-config].globals.ts-jest.diagnostics` option):
    src/internal/internetReachability.ts:91:13 - error TS2322: Type 'Timeout' is not assignable to type 'number'.

    91             this._currentTimeoutHandle = setTimeout(
                   ~~~~~~~~~~~~~~~~~~~~~~~~~~
    src/internal/internetReachability.ts:101:11 - error TS2322: Type 'Timeout' is not assignable to type 'number'.

    101           this._currentTimeoutHandle = setTimeout(
                  ~~~~~~~~~~~~~~~~~~~~~~~~~~

 FAIL  src/__tests__/eventListenerCallbacks.ts
  ● Test suite failed to run

    TypeScript diagnostics (customize using `[jest-config].globals.ts-jest.diagnostics` option):
    src/internal/internetReachability.ts:91:13 - error TS2322: Type 'Timeout' is not assignable to type 'number'.

    91             this._currentTimeoutHandle = setTimeout(
                   ~~~~~~~~~~~~~~~~~~~~~~~~~~
    src/internal/internetReachability.ts:101:11 - error TS2322: Type 'Timeout' is not assignable to type 'number'.

    101           this._currentTimeoutHandle = setTimeout(
                  ~~~~~~~~~~~~~~~~~~~~~~~~~~

Test Suites: 2 failed, 2 total
Tests:       0 total
Snapshots:   0 total
Time:        7.64s

@lonnylot
Copy link
Contributor Author

Please note the second commit (and all changes to internetReachability.js) were forced by the linter in order for me to push...

@lonnylot
Copy link
Contributor Author

I've modified the TypeScript definitions and believe what I've added is correct. It now passes my local npm run validate:typescript

@matt-oakes
Copy link
Collaborator

Thanks for this. It looks like the lint errors were something to do with configuration your machine. I was getting errors with the changes you made there. I've reverted them and will merge the branch when CircleCI is happy with it all 👍

@matt-oakes matt-oakes merged commit e3fc1b4 into react-native-netinfo:master Dec 20, 2019
react-native-community-bot pushed a commit that referenced this pull request Dec 20, 2019
## [5.0.1](v5.0.0...v5.0.1) (2019-12-20)

### Bug Fixes

* Ensure passing no configuration to the hook works correctly ([#264](#264) by [@lonnylot](https://github.com/lonnylot)) ([e3fc1b4](e3fc1b4)), closes [#262](#262)
@react-native-community-bot
Copy link
Collaborator

🎉 This PR is included in version 5.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@lonnylot lonnylot deleted the issue/remove-default-config branch December 23, 2019 14:32
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.

3 participants