react-native-netinfo / react-native-netinfo

React Native Network Info API for Android & iOS
MIT License
1.94k stars 391 forks source link

new NativeEventEmitter() was called with a non-null argument [needs PR! good first issue!] #486

Open lubomyr opened 3 years ago

lubomyr commented 3 years ago

Environment

System: OS: macOS 11.5.2 CPU: (8) arm64 Apple M1 Memory: 152.89 MB / 16.00 GB Shell: 5.8 - /bin/zsh Binaries: Node: 15.14.0 - ~/.nvm/versions/node/v15.14.0/bin/node Yarn: 1.22.10 - /usr/local/bin/yarn npm: 7.7.6 - ~/.nvm/versions/node/v15.14.0/bin/npm Watchman: Not Found Managers: CocoaPods: 1.10.1 - /Users/lyubomyr/.rbenv/shims/pod SDKs: iOS SDK: Platforms: iOS 14.5, DriverKit 20.4, macOS 11.3, tvOS 14.5, watchOS 7.4 Android SDK: API Levels: 29, 30 Build Tools: 28.0.3, 29.0.2, 30.0.2, 30.0.3 System Images: android-30 | Google APIs ARM 64 v8a, android-S | Google APIs ARM 64 v8a, android-S | Google Play ARM 64 v8a Android NDK: Not Found IDEs: Android Studio: 2020.3 AI-203.7717.56.2031.7621141 Xcode: 12.5.1/12E507 - /usr/bin/xcodebuild Languages: Java: 1.8.0_282 - /usr/bin/javac npmPackages: @react-native-community/cli: Not Found react: 17.0.2 => 17.0.2 react-native: 0.65.0 => 0.65.0 react-native-macos: Not Found npmGlobalPackages: react-native: Not Found

Platforms

Issue appears only on Android Platform

Versions

Description

With newest react-native version 0.65.0 which was released few days ago, on android platform when run app i can see warnings: WARN new NativeEventEmitter() was called with a non-null argument without the required addListener method. WARN new NativeEventEmitter() was called with a non-null argument without the required removeListeners method. These warnings not appeared on react-native 0.64.2 and older versions, also not appear on IOS platform

Reproducible Demo

Just create new template of react-native project and add import NetInfo from "@react-native-community/netinfo";

const unsubscribe = NetInfo.addEventListener(state => { console.log("Connection type", state.type); console.log("Is connected?", state.isConnected); });

mikehardy commented 3 years ago

We need a PR for it - can probably be something like https://github.com/invertase/react-native-firebase/pull/5616/files#diff-c887076b0c80d540aed1bfbb472cc8f20516634e8e72d53014c65d690bba4fb1

Please make a est it to see if that resolves the issue and make a PR?

lubomyr commented 3 years ago

@mikehardy, Yes it resolved issue, I made PR. Thanks. I noticed another rn-libraries which need similar fixes.

kneza23 commented 3 years ago

Will there be a new release for this fix?

mikehardy commented 3 years ago

@kneza23 I haven't see activity from @matt-oakes in forever here so I am not sure. Until then what you want is https://github.com/ds300/patch-package to ingest the change from #487 - or you can realize it is just a warning, and wait.

matt-oakes commented 3 years ago

@mikehardy Thanks for the prompt. To be honest, I hadn't realised that I was the only one with merge powers on this repo. I thought everyone else who had access still would when this was moved outside the community repo. I have sent you an invite.

I don't really use this library too much so I don't have too much time for it these days. I should have some time at the end of September to look into open source stuff so I'll try to tidy up the repo a bit then.

Thanks for helping out and commenting on peoples issues in the meantime!

mikehardy commented 3 years ago

Yeah as the RNCommunity thing was re-focused it purged out user powers - I saw the invite and accepted it. I have a lot on my plate as well but can help with emergency stuff, keeping the lights on etc :-)

Can I ask how publishing is done? If the maintainer work was reduced to PR review+merge (with all the test / publish stuff done fully-auto) that would make this a lot easier to deal with. It's been a huge benefit keeping device-info going that I can trust CI during PR review and after I merge it just releases itself

matt-oakes commented 3 years ago

@mikehardy In theory releasing should be handled by semantic release and CircleCI, but for some reason that seems to be broken at the minute. I'll try to look at that this week so it can be done by anyone. I'll likely move it over to Github Actions šŸ‘

keshavsharmalws-zz commented 2 years ago

i m getting same error warning can i import this netinfo library in same project to fix this or should i create a new project?

warning

reandov commented 2 years ago

Is the problem fixed? I'm using the latest version of the library and still getting these warnings

mikehardy commented 2 years ago

@evnrodr have you checked the releases / changelog to see the history on it? (tl;dr: fixed, but then reverted as it had unintended blocker side effects while the warning is just a warning)

reandov commented 2 years ago

Thanks for the info, didn't saw it was reverted, sorry šŸ˜…

kyelewis commented 2 years ago

Is there a new issue tracking this?

mikehardy commented 2 years ago

No, not that I'm aware of - as it's a warning it fell to a lower priority for me, so I haven't personally spent any time on it and probably won't be able to. Would love to see a PR

kyelewis commented 2 years ago

I see, it was only reverted for iOS- not android. No problems then I think.

MuhammadTahir687 commented 2 years ago

WARN new NativeEventEmitter()was called with a non-null argument without the requiredaddListenermethod. WARNnew NativeEventEmitter()was called with a non-null argument without the requiredremoveListeners` method.

how to solve it

zoolle commented 2 years ago

I see, it was only reverted for iOS- not android. No problems then I think.

It is on android as well

mh-hridoy commented 2 years ago

It's showing the same warning with @react-native-mapbox-gl/maps package

yongyiwong commented 2 years ago

same warning

WARN new NativeEventEmitter() was called with a non-null argument without the required addListener method. WARN new NativeEventEmitter() was called with a non-null argument without the required removeListeners method. WARN new NativeEventEmitter() was called with a non-null argument without the required addListener method. WARN new NativeEventEmitter() was called with a non-null argument without the required removeListeners method. WARN new NativeEventEmitter() was called with a non-null argument without the required addListener method. WARN new NativeEventEmitter() was called with a non-null argument without the required removeListeners method.

mikehardy commented 2 years ago

@yongyiwong it is as if you did not read any of the comments above :point_up: :thinking: - it's expected that this warning is still emitted.

mikehardy commented 2 years ago

I suppose it's best to reopen though, as the PR had to be reverted

yongyiwong commented 2 years ago

Thanks @mikehardy . hope it will be reverted for android, too.

pke commented 2 years ago

What is this warning about even? RN error/warning could also be improved and specify the nativeModule that is causing the warning. I think I'll file an issue over at RN.

So Mike you are saying 12.7.1 is unstable to use? Which version reverted the warning?

mikehardy commented 2 years ago

The changelog is always the guide: https://github.com/react-native-netinfo/react-native-netinfo/blob/master/CHANGELOG.md#602-2021-09-03 - there is no 12.7.1 version here, that sounds like a react-native-firebase version, we're on 13.0.1 over there at the moment

The warnings aren't that bad with regard to specificity, it's just that they are not that prescriptive with regard to how to actually handle them, and since they aren't causing an issue (unless we just put in stubs...) and no one appears to have time to dig in (I certainly don't at the moment), they'll keep popping up

mikehardy commented 2 years ago

@yongyiwong I do not believe there is a revert needed for android, android appears to be working fine, and with #548 it will manage it's own listener count and should behave correctly.

As noted in #548 during review I found the root cause of why #487 caused a regression here but works in other modules. Other modules must not be checking observer count somehow or they have called super class methods correctly

I re-reviewed this and I'm really sad to see there is no general infrastructure in the Java native modules for managing observer count. The counts are maintained in javascript on the other side of the bridge. The same is only partially true for the iOS side - your link there combined with a re-review of the react-native core code solved the mystery I had of why #487 caused #492 and needed a revert in #493 - we partially override the superclass listener counting infrastructure there by using isObserving but without managing observer status as listeners are added/removed. We need something on the iOS side more like what you've implemented here.

So a PR here to silence this warning requires:

Would love a PR for this - the code change is easy but the big contribution will be that you do the quick/easy code change and test it :-)

layeng commented 2 years ago

Hi I have faced this issue since Jan 2022. I saw in the my code that the 2 listeners are available but the problem still happens. Any solution or just ignore the warnings?

@ReactMethod public void addListener(String eventName) { // Keep: Required for RN built in Event Emitter Calls. mConnectivityReceiver.hasListener = true; }

@ReactMethod public void removeListeners(Integer count) { // Keep: Required for RN built in Event Emitter Calls. if (count == 0) { mConnectivityReceiver.hasListener = false; } }

DrOverbuild commented 2 years ago

Would love a PR for this - the code change is easy but the big contribution will be that you do the quick/easy code change and test it :-)

What's the status of this issue?

Using RN 0.67.3, I'm not getting the warning on iOS. It seems we do not need to implement addListener/RemoveListeners methods in the native iOS module anymore. So this issue could probably be closed.

mikehardy commented 2 years ago

Using RN 0.67.3,

I'd double-check your local environment. I'm not aware of this requirement to implement addListener/removeListeners going away

DrOverbuild commented 2 years ago

Ah, looking at RN source, you are right about the requirement not going away. Nothing about addListener/removeListeners has changed since the warnings were added in 0.65.

The base class RCTEventEmitter already implements and exports the methods, so the warnings should never be fired on iOS. I can't reproduce the warnings even with a bare react-native init project with netinfo.

The implementation of those methods in RCTEventEmitter maintains a counter _listenerCount. When a listener is added, the counter is incremented. If _listenerCount is now 1, startObserving is called. When listeners are removed, _listenerCount is decremented by the number of listeners removed, and if the count reaches zero, stopObserving is called. As ouabing mentioned in #548, RNCNetInfo already has been optimized to not emit an event if no one is listening.

I was going to help close this issue with a PR but after digging into it I don't see that anything needs to be done on the iOS platform because the behavior matches the Android platform.

edritech93 commented 2 years ago

same issue for me

mikehardy commented 2 years ago

@edritech93 advance the conversation or leave a reaction, "me too" comments are actually a negative, they generate notifications which require attention from maintainers is a form of time theft for already chronically time-short maintainers. Thanks

JRGunthner commented 2 years ago

I need to solve this problem. Any solution?

mikehardy commented 2 years ago

@JRGunthner so do I! Looking forward to your PR.

JRGunthner commented 2 years ago

Putting this, at least removed the warnings but it's still not the best solution:

import { LogBox } from 'react-native'; LogBox.ignoreLogs(['new NativeEventEmitter']); // Ignore log notification by message LogBox.ignoreAllLogs(); //Ignore all log notifications

russeg commented 2 years ago

@JRGunthner so do I! Looking forward to your PR.

Why are you asking him, did he introduced the problem????

@mikehardy I noticed you seem to like to tell people to do PR when they have a problem. Well, how about not putting in the bug in the first place?

I know what you are tying to do with this "looking forward to your PR" nonsense of yours. Not really passive aggresive in my book, but pretty dumb nonetheless. And I will call you out on it whenever I see a post of yours like this.

mikehardy commented 2 years ago

Dude, I'm the only human left willing to read through PRs here and in a few other repos. I'm the only one left here with permission to hit the release button. I'm not being passive aggressive I'm just saying if anyone wants this library to move forward

Do the work

I'll collaborate and help merge and release, but I don't have the energy to fight problems that don't affect my actual use cases.

Call out whatever you want, but ideally, do the work.

mikehardy commented 2 years ago

You might like: https://hackernoon.com/i-thought-i-understood-open-source-i-was-wrong-cf54999c097b

HarshMB commented 2 years ago

My app also crashed with same error in iOS simulator

ERROR Invariant Violation: new NativeEventEmitter() requires a non-null argument.

I am using below versions:- react-native : v0.66.4 @react-native-community/netinfo : v6.0.1

For latest version also same error so downgraded but got same issue

Screenshot 2022-05-31 at 6 41 59 PM

mikehardy commented 2 years ago

@HarshMB you have some local integration issue, if you try the example here it will work, indicating that the module is working

jeafgilbert commented 1 year ago

Updating @react-native-clipboard/clipboard to v1.11.2 fixes the issue. Thanks @chrisglein.

mikehardy commented 8 months ago

@cunguyendev +1 comments are worse then useless, you waste everyone's time with useless notifications that do not move anything forward.

Post a PR, or just do a reaction on the main issue instead of wasting people's time.