Skip to content

Conversation

@cwise89
Copy link
Owner

@cwise89 cwise89 commented Jan 21, 2021

Implementation of useOnlineEffect. No tests yet to confirm on the correct path lol.


if (
(typeof pollingConfig === 'object' && pollingConfig.enabled === true) ||
(needsPolling === true && typeof pollingConfig === 'object')

Choose a reason for hiding this comment

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

In case pollingObject.enabled === false, this would return an incorrect object

{
    enabled: false,
    url: 'https://ipv4.icanhazip.com/',
    timeout: 5000,
    interval: 5000,
}

Change to (needsPolling === true && typeof pollingConfig === 'object' && pollingConfig.enabled === undefined)

};

export const useOnlineEffect: UseOnlineEffectType = (
callback,

Choose a reason for hiding this comment

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

Better than using a callback would be use useState.

const [isOnline, setIsOnline] = useState(true);

const goOnline = () => {
    setIsOnline(true);
};

const goOffline = () => {
    setIsOnline(false);
};

...
setInterval(() => {
        ping(pingConfig).then(setIsOnline);
      }, interval);
...

return isOnline;

A user can then just use it like:

const isOnline = useOnlineEffect();

useEffect(() => {
  if (isOnline) { 
    console.debug("we're online");
  } else {
    console.warn("we're offline");
  }
}, [isOnline])

Choose a reason for hiding this comment

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

If callback is still wanted, you could add

if (callback) {
  useEffect(() => {
    callback(isOnline);
  }, [isOnline])
}

to have both options.

src/index.tsx Outdated
if ((mustPoll || enabled) && 'url' in pingConfig) {
const { url, timeout, interval } = pingConfig;

setInterval(() => {

Choose a reason for hiding this comment

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

assign to intervalId

window.removeEventListener('online', goOnline);
window.removeEventListener('offline', goOffline);

if (mustPoll || enabled) {

Choose a reason for hiding this comment

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

use if (intervalId) instead. It will only be set in this case.

src/index.tsx Outdated
let intervalId: number | undefined;

// if we are polling for online status, set up the setInterval.
if ((mustPoll || enabled) && 'url' in pingConfig) {

Choose a reason for hiding this comment

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

Shouldn't getPollingConfigs have taken care to set enabled correctly.
Here you are overriding the choice of the user.
IMO enabled should suffice.

src/index.tsx Outdated
ping({
url,
timeout,
}).then(online => (online ? goOnline() : goOffline()));

Choose a reason for hiding this comment

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

Could be simplified to .then(callback)

@alexanderadam
Copy link

Thank you so much for your work on this. 🙌
Is there any rough release date for v3? I saw someone asking this in an issue but the user closed it without a hint.

cwise89 and others added 2 commits August 28, 2021 17:06
* Created hashed object that we can store to the window to make sure that duplicates are not being ran.

* Added the unmount delete from the window.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants