Implement Watchers (fixes #107, #111). r=gmarty #129
Conversation
|
Hey @gmarty r?, I think the code part is ready for review, could you please take a look? I'm just planning to add few more tests. |
| } | ||
|
|
||
| return Promise.all(pingPromises); |
There was a problem hiding this comment.
I think we have a similar issue here than when trying to put these 2 pings in a promise.
If one of the ping takes very long to resolve, the next ping will be delayed for as long. Maybe it's better to have 2 timers here: 1 for local and 1 for remote.
What do you think?
There was a problem hiding this comment.
I think we have a similar issue here than when trying to put these 2 pings in a promise.
If one of the ping takes very long to resolve, the next ping will be delayed for as long.
It's hard to say, preliminary ping takes very long to resolve should be an edge case, and more important to have less network requests as possible.
Maybe it's better to have 2 timers here: 1 for local and 1 for remote.
What do you think?
Yeah, maybe we can try that, I'm wondering maybe they should even have different periods... Do you want me to try to do it in this PR or file appropriate issue?
There was a problem hiding this comment.
Yes please can you try to do that in that PR?
Also different periods of time sound good.
Maybe also we should use the connection type from the net-info API (only in Chrome for now). If on Wifi we can ping more often that on radio. This optimisation can definitely wait for another PR as long as we fill a bug for it.
There was a problem hiding this comment.
Yes please can you try to do that in that PR?
Okay, trying.;
Also different periods of time sound good.
Maybe also we should use the connection type from the net-info API (only in Chrome for now). If on Wifi we can ping more often that on radio. This optimisation can definitely wait for another PR as long as we fill a bug for it.
Done #135
|
I'm very excited about this PR, it shows the real potential of a powerful and user-friendly API! I left some comments, but r+! |
|
|
||
| assert.isDefined( | ||
| component, | ||
| 'ReactDOM.render() did not render component (see issue #133).' |
There was a problem hiding this comment.
Can you use the link, an issue number only can be ambiguous (is it a foxbox error? a React error?):
https://github.com/fxbox/app/issues/133
|
Thanks a lot for review! Review comments handled and travis is green, merging! |
No description provided.