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

Another bug in 2.15 #788

Closed
StasD opened this issue Oct 20, 2019 · 13 comments · Fixed by #790
Closed

Another bug in 2.15 #788

StasD opened this issue Oct 20, 2019 · 13 comments · Fixed by #790

Comments

@StasD
Copy link

StasD commented Oct 20, 2019

Issues a warning:

Sending `DownloadBegin` with no listeners registered.

when I do

  let {promise} = fs.downloadFile({fromUrl, toFile});
@StasD
Copy link
Author

StasD commented Oct 20, 2019

Going back to 2.14 for now...

@itinance
Copy link
Owner

@StasD
So, it's just a warning and not a serious error? Will definitely have a look at it. I'm just asking to identify if its a serious issue breaking downloadFile

@StasD
Copy link
Author

StasD commented Oct 21, 2019

It's a warning, but it is displayed very prominently when I run the program in iOS simulator, and I have to press "Dismiss All" before I can use the program in the simulator again.
Just noticed there is another warning about "DownloadProgress" listener.
So, these are very annoying and also not necessary. I don't care about those listeners whatsoever, and if I did, I would have found how to use them in the documentation.
It would be great if you remove these warnings or downgrade them so that they don't show up as prominently in the simulator. Thanks.

@itinance
Copy link
Owner

I think it has something to do with this PR: #752

@itinance
Copy link
Owner

@jnpdx can you have a look at this issue here pls? There is little chance that it was introduced with #752

@StasD
Copy link
Author

StasD commented Oct 21, 2019

Also I use react-native-navigation component, which requires AppDelegate.m to be modified: here. I took a quick look at "Files changed" section of that PR and thought this information could be relevant...

@jnpdx
Copy link
Contributor

jnpdx commented Oct 21, 2019

@itinance

It looks like event listeners are only registered in the event that a begin parameter is passed. So, it seems that the options would be:

  1. Register an empty callback with the emitter in the event that no begin parameter was sent.
  2. Only send events with the emitter if the begin or similar parameters have been registered.

#1 seems like a better option because #2 will require changing the signature of the native methods in order to implement. I'd be happy to submit a PR for #1.

However, I have a secondary issue, which is I can't actually seem to replicate this issue, whether I pass callbacks or not.

@StasD -- what versions of React Native and iOS are you on?

@StasD
Copy link
Author

StasD commented Oct 21, 2019

All latest: RN 0.61.2, iOS 13.1 in the simulator.

@StasD
Copy link
Author

StasD commented Oct 21, 2019

@itinance Also, please check the above link from react-native-navigator documentation. I have a suspicion that the way how AppDelegate.m is modified might have something to do with this. Though I might be wrong.

@jnpdx
Copy link
Contributor

jnpdx commented Oct 21, 2019

@StasD Ah, I see, this issue only appears for me in certain scenarios (eg other listeners have never been registered). Since I was testing uploads as well, I didn't see it.

It looks like the current hot fix would be to pass () => {} for the begin parameter when you call downloadFile. (@StasD can you confirm that works with your setup as a temporary fix?)

That also supports my first fix idea I suggested above. I'll submit a PR unless @itinance has an issue with that method of fixing it.

@jnpdx
Copy link
Contributor

jnpdx commented Oct 21, 2019

@StasD alternately, try with this fork, which will become my PR if it works: https://github.com/jnpdx/react-native-fs#7558d9f

@StasD
Copy link
Author

StasD commented Oct 21, 2019

Yes, if I pass () => {} for the begin parameter, warnings disappear. Interestingly, the warning for progress disappears too...

@jnpdx
Copy link
Contributor

jnpdx commented Oct 21, 2019

@StasD yeah, I think that's why I didn't see this issue at first the emitter is really inconsistent about when it complains about things. I'll submit my patch as a PR.

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

Successfully merging a pull request may close this issue.

3 participants