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

Support short polling #418

Open
klis87 opened this issue Dec 21, 2020 · 21 comments
Open

Support short polling #418

klis87 opened this issue Dec 21, 2020 · 21 comments

Comments

@klis87
Copy link
Owner

klis87 commented Dec 21, 2020

Sometimes it might be handy to refresh query data, which could allow to have more "real-time" user experience. This is how I would go about it:

  1. To RequestAction add meta.pool: number (in seconds), this would be an interval, how often queries should be refetched automatically
  2. We could implement it as a separate middleware (probably it would go even as 1st one in the stack)
  3. Of course this middleware should be disabled when running with ssr: 'server'
  4. PollingMiddleware would just pass all actions through, but also it would check for queries with meta.poll, for which it would activate setInterval, repeating those actions, dispatching them again after x seconds
  5. I think that dispatched query from this new middleware could also add meta.pollCounter: number, it could show us which time a given query is repeated, and also this could prevent poll looping - we shouldn't create interval again for a query which is result of this middleware, otherwise we would have multiple intervals for the same query
  6. Making a new request of the same type (type + requestKey) without meta.pollCounter should clear current interval (it really means than a new query was dispatched, not from middleware but outside), and, depending on meta.poll, possibly set new interval
  7. we should add stopPolling action which should have the same arguments like for example abortRequests
  8. probably new flag should be added to resetActions but for now I won't provide details as resetActions is work in progress on pure-react branch
@favger
Copy link

favger commented Dec 25, 2020

That would be really amazing. My current usage pattern;

ss-redux-requests-c

@klis87
Copy link
Owner Author

klis87 commented Dec 30, 2020

@favger Thx for the snippet. I can see you use React, so you might be interested that additionally stopPolling callback will be returned by useQuery, also unmounting useQuery, changing type or requestKey will reset query, and resetting will also stop polling if any

@iwan-uschka @favger the only thing I am wondering, should we only have meta.poll to fire request which will be auto polled, or maybe we should also have a dedicated action which starts polling for an already present query? Personally I am really into this assymetry, starting just with dispatching query with meta.poll (no startPolling action), and stopping either by:

  1. resetting
  2. stopPolling action dispatching
  3. firing the very same query again, as point 6 in the task requirements.

I don't wanna introduce startPolling action, because this is not as simple, usually we would need variables to request action, and I don't wanna store them for future usage like startPolling action.

Theoretically though, we could introduce startPolling callback in useQuery, under the hood it would just dispatch query with passed variables (this will be new prop in the future release in useQuery) and added meta.poll automatically. However, I am not sure it is worth doing.

@iwan-uschka
Copy link

iwan-uschka commented Dec 31, 2020

@klis87 I cannot think of any use case right now where startPolling callback would be needed. We have total control when to dispatch the specific action. If i need the same request type separately as a one-time action and a loop action i can create 2 actions separately or dispatch the same action using different parameters.
In this case i think we don't even need any stopPolling callback. We should just use abortRequests (@redux-requests/core) as before. Or do you think it's necessary to abort manually a single request execution within a loop without aborting the loop itself?

I had to create an async loop mechanism this year myself. I just created a gist you can look at. I am using an iterator function and we can configure the behaviour like this

// config.interval.type
// - strict: cancel previous run if necessary after timeout (interval duration) and start a new one
// - placid: wait for previous run to complete and start timeout (interval duration) afterwards
// - efficient: wait for previous run to complete and wait for rest of timeout (interval duration) if necessary

Perhaps for inspiration :)

And @favger: checking isWindowActive is a smart thing to do during polling. Thanks for sharing. I'll take this into account for my next projects. Just one comment: you could also run clearInterval on isWindowActive===false. This way no data is fetched in the background.

@favger
Copy link

favger commented Dec 31, 2020

@klis87 I trust your feelings about this. My ability to analyze is not very good.

Maybe the SWR's settings can help us get an idea.

Good work

@klis87
Copy link
Owner Author

klis87 commented Dec 31, 2020

@iwan-uschka Thx for the feedback. Regarding abortRequest, I prefer to keep this only to just abort requests, no matter whether polling is used or not. Hence I think dedicated action stopPolling would be cool. However, resetRequests should stop all polling, I cannot think about any case which would reset a request just to make another with polling. And thanks for the gist :)

@favger actually very cool idea with isWindow active, we could have actions like pausePolling and resumePolling! This is probably nice to have TODO, for the future, but it makes sense not to pool when sth is not visible!

@klis87
Copy link
Owner Author

klis87 commented Jan 13, 2021

Just in case, if anyone started working on this, please let me know, as probably this is the next on my list, as I just published new version focused on React. If not, that's totally fine, I just don't want to duplicate our efforts :)

@klis87
Copy link
Owner Author

klis87 commented Jan 13, 2021

@iwan-uschka @favger btw, I am interested on your opinion about #432 , I would really appreciate your feedback!

@klis87
Copy link
Owner Author

klis87 commented Jan 17, 2021

@iwan-uschka @favger released in https://github.com/klis87/redux-requests/releases/tag/%40redux-requests%2Fcore%401.5.0

note that I didn't implement pause and resume polling actions, because it can be just achieved by dispatching stopPolling and then, in a proper time by dispatching a given query with meta.poll again

@klis87 klis87 closed this as completed Jan 17, 2021
@favger
Copy link

favger commented Jan 17, 2021

@klis87 I tested it, it works fine. How would you recommend not to stop automatic renewal, except for the browser tab only?

@klis87
Copy link
Owner Author

klis87 commented Jan 17, 2021

@favger you mean to stop polling when a browser tab is not active?

@favger
Copy link

favger commented Jan 17, 2021

@klis87 Yes, definitely

@klis87
Copy link
Owner Author

klis87 commented Jan 17, 2021

@favger probably you would need to use onEffect in your react component, listen to tab window events and:

  • dispatch stopPolling from useQuery response on disactivating tab
  • dispatch load from useQuery response on activating tab

if this works, we could think of an extra prop like disablePolling: boolean for useQuery, and even for RequestsProvider - then we could set it globally

@favger
Copy link

favger commented Jan 17, 2021

@klis87 I already use a similar structure as you said, but having a shorter method may be more comfortable for the developer.

Thank you very much for your support.
Good work.

@klis87
Copy link
Owner Author

klis87 commented Jan 17, 2021

@favger Thank you as well for contributing with valid issues and ideas!

Once you set this up, please share some snippet, so I will make this easier to support!

@favger
Copy link

favger commented Jan 17, 2021

We can solve it this way;

/*
 * Action Types
**/
export const SET_WINDOW_ACTIVE = "APP WINDOW ACTIVE STATE";

/*
 * Action Creators
**/
export const setWindowActive = state => ({
    type: SET_WINDOW_ACTIVE,
    payload: state
});
/*
 * Action Reducers
**/
case SET_WINDOW_ACTIVE: {
    const isWindowActive = action.payload === "toggle" ? !state.isWindowActive : action.payload;
    return {
        ...state,
        isWindowActive
    }
}
// [WINDOW]: Focus (Event)
useEffect(() => {
    if (document.hidden) dispatch(setWindowActive(false));
    const onWindowCheckFocus = (e) => e.type === "visibilitychange" && dispatch(setWindowActive("toggle"));
    document.addEventListener("visibilitychange", onWindowCheckFocus);
    return () => document.removeEventListener("visibilitychange", onWindowCheckFocus);
}, [setWindowActive]);

meta.poll= 10
meta.pollWhenHidden = true/false

  • if pollWhenHidden is false, the poll will not work while the window is invisible.

I hope that has been revealing.

@favger
Copy link

favger commented Jan 17, 2021

SS-redux-requiest-iswindowactive

Maybe we can show it that way.

@klis87
Copy link
Owner Author

klis87 commented Jan 17, 2021

@favger ok, so you have a global way to get this attr. So, I guess extra prop to useQuery like pausePolling={!isWindowActive} would do it. If this is true, then:

  1. useQuery would remove poll from meta on mount
  2. useQuery would fire stopPolling when pausePolling is switched from false to true
  3. useQuery would refetch (if autoLoad is true when pausePolling is switched from true to false), without stripping meta.poll ofc

Adding the same in RequestsProvider would be also cool. The only issue is that you store this value in store, which can be created by RequestsProvider itself. Then probably we would need to use a callback for provider pausePolling prop.

Perhaps this actually should be solved on Redux layer (in the core)...

For now I am pretty sure you could create useQuery wrapper with above logic for code reusability

@klis87 klis87 reopened this Jan 17, 2021
@favger
Copy link

favger commented Jan 17, 2021

@klis87 excellent

@iwan-uschka
Copy link

Hope it's not too late to join the party. Installed & tested [email protected]. Works great 👍. But there is a "but" :)

I want to make a suggestion for an improvement because as a developer i probably want to handle requests differently depending on the use case and respecting potentially network issues. Before giving some examples i would like to give a quick reminder of the configuration matrix i mentioned at the beginning of this thread:

// config.interval.type
// - strict: cancel previous run if necessary after timeout (interval duration) and start a new one
// - placid: wait for previous run to complete and start timeout (interval duration) afterwards
// - efficient: wait for previous run to complete and wait for rest of timeout (interval duration) if necessary

@klis87 , you implemented the strict option. A use case i can think of could be something like poll: 30. If a request hasn't completed after 30 seconds it can be treated as a timeout without any doubt. So it can be killed and a new request can be started.

Another example: short timed polling, for instance poll: 2. User has to deal with bad network conditions. The request takes more than 2 seconds. Hypothetically, i know :) In this case the user won't get any response because the request is killed before a response can be received completely. In this case i would prefer to use the option i call placid or efficient. A running request should not be killed (using axios you can define a timeout additionally which will kill the request no matter what). The app should wait for the response before starting a new request.

@klis87 , if you like i could come up with a proposal or pull request including these options. We can discuss about the naming of course, or if placid is needed next to efficient or the other way around. Only having the strict option like it is implemented right now, i couldn't use poll mechanism unfortunately.

@klis87
Copy link
Owner Author

klis87 commented Jan 17, 2021

@iwan-uschka I don't have enough time to analyze your post tonight long enough unfortunately, I will try to do it tomorrow, but I will answer now as you mentioned very important topic which I feel is related to sth else and will be compatible with your suggestion. See #416 for my effects proposal, for example with takeLeading effect if meta.poll is 10, but previous request is still pending, then new one would just join the previous one :) but perhaps polling should have dedicated options like you said, no matter what effect is set. But I sense that effects would be enough here as well.

Anyway, right now I am working on subscriptions, which probably will be used more often than polling anyway for real-time functionality.

@klis87
Copy link
Owner Author

klis87 commented Jan 20, 2021

@iwan-uschka I analyzed your interval type suggestion and I think that:

  • strict is the same as takeLatest - we already have it
  • efficient is the same as takeLeading
  • placid - is very similar to efficient, it would also use takeLeading, but timeout is shifted depending on the response time of a previous request

Is it necessary to support both efficient and placid?

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

No branches or pull requests

3 participants