metalk8s icon indicating copy to clipboard operation
metalk8s copied to clipboard

UI: improve "refresh" sagas

Open carlito-scality opened this issue 6 years ago • 2 comments

Component: UI

Why this is needed:

  • In the UI, to refresh resources in a page we call "refresh" sagas that look like:
export function* refreshAlerts() {
  yield put(updateAlertsAction({ isRefreshing: true }));
  const resultAlerts = yield call(fetchAlerts);
  if (!resultAlerts.error) {
    yield delay(REFRESH_TIMEOUT);
    const isRefreshing = yield select(
      state => state.app.monitoring.alert.isRefreshing,
    );
    if (isRefreshing) {
      yield call(refreshAlerts);
    }
  }
}

and to stop the refresh, we call "stopRefresh" sagas that look like:

 yield put(updateAlertsAction({ isRefreshing: false }));

This "refresh" management is split in 2 different sagas, so it is hard to understand. Moreover, the "refresh" saga has to wait REFRESH_TIMEOUT seconds (since delay is a blocked call) to know if it has to relaunch the refresh based on a redux state isRefreshing.

Suggested improvements (cf @gdemonet ):

export function* refreshAlerts() {
  while (true) {
    yield take(REFRESH_ALERTS);
    while (true) {
      yield fork(fetchAlerts);
      const {interrupt, requeue} = yield race({
        interrupt: take(STOP_REFRESH_ALERTS),
        requeue: delay(DELAY_TIMEOUT),
      }); 
      if (interrupt) { break; }
    }
  }
}
  • if there is an error during fetchAlerts, we can just yield put({type: STOP_REFRESH_ALERTS}), to stop the refresh in the fetchAlerts saga.
  • It would be even better to cancel "fetchAlerts" when "interrupt" occurs.
  • Consequently, state.app.monitoring.alert.isRefreshing is only used to display refresh icon.

What should be done:

Implementation proposal (strongly recommended):

Test plan:

carlito-scality avatar Nov 12 '19 16:11 carlito-scality

Is this still relevant with React Query @JBWatenbergScality?

gdemonet avatar May 05 '21 16:05 gdemonet

Not really we should better have a debt ticket about switching the current implementation to react-query 🤔

JBWatenbergScality avatar May 05 '21 16:05 JBWatenbergScality