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

feature/onParamsActionsCompleted #33

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JonHaywood
Copy link

Adds an onParamsActionsCompleted action creator which is dispatched after all params action creators are dispatched.

This is useful for situations where different you need to issue one final action after all param actions have run and the redux store is synced with the query string. For example, say pageNumber and sortBy both have their associated actions which update the redux store. However after those run you need one final action - say refetchData - to run after both of those items have been updated.

…er all params action creators are dispatched.
@Treora
Copy link
Owner

Treora commented Apr 11, 2019

Thanks for the suggestion. The use case seems completely valid, you may want to know when all URL parameter changes have been handled. Our current approach is lacking, as it just dispatches the actions one by one, without e.g. indicating their relatedness nor telling how many there will be in total. However, I wonder whether your solution, i.e. dispatching an additional action to signal that the URL parameter changes hawe finished, is the best solution to this.

On the upside, it is intuitive: a signal that the URL parameters have changed. Simple.

On the downside, it feels like it encourages bad code structure (though I may be too opinionated): in your example refetchData should ideally be invoked because the state's values for pageNumber and/or sortBy have changed. You might end up adding another input that can change the page number, and then you would have to ensure you don't forget to call refetchData again; not great.

If I understand you correctly, the problem currently is that if you would react both to a change in pageNumber or sortBy, you would trigger refetchData twice, which would be wasteful (unless it is throttled). I have a hunch this problem is better approached differently: conceptually, you want the ability to regard the change of URL parameters as a single action, not a series of actions. This action will cause several state variables to be updated, at once.
This idea came up in #18:

One other solution that comes to mind is to rather let the user specify a single action, which will get an object with all changed(?) params. You would specify the action in the top-level config rather than in each of the params configs. Not sure if this is a nice API, or what would be the desired semantics if actions are specified both at the top-level and for some individual parameters. But it seems a simple solution to give full freedom in how to handle location updates.

Would such an approach seem equally helpful to you, or inconvenient?

A remaining issue, raised in the quote above: how to make a nice API for this. I imagine an option (e.g. bulkHandlerAction, better name needed), which upon location change is invoked once with a list of changes, e.g. { pageNumber: { old: 2, new: 3 }, sortBy: { old: 'name', new: 'date' } }. It then returns a single action that is to be dispatched, which could be used to update the whole state at once.

One question that this idea also raised: Can one specify both a bulk handler, as well as individual action creators per parameter? Would those actions then be executed before the bulk handler, and the bulk handler only gets a list of remaining changes? Perhaps it is easiest to say instead that if the bulk handler is defined the individual parameter's actions are not dispatched. (for advanced usage we could pass those individual actions to the bulk handler as a second argument, which could then return a thunk that dispatches each of them; assuming redux-thunk is used).

@JonHaywood
Copy link
Author

JonHaywood commented Apr 15, 2019 via email

@Treora
Copy link
Owner

Treora commented Apr 15, 2019

So.. sounds like either solution would work for you? I am still poised towards the bulk hander approach, especially as it may also enable many other types of customisation. I like your suggestion to pass, for each (changed) parameter, the handler the changes and actions together in one object. Though I'd probably concatenate these objects in an object, not in an array:

{
  paramName: {
    old // old value
    new // current value
    action // if the parameter defined an action creator, the action it created.
           // (which has not been dispatched: the bulk handler could do so using a thunk).
  },
  // ....
}

In case you would like to work this out into a PR, you are most welcome; but of course no worries if not. I likely will not get around to it soon (partly since this module lacks tests, so it's a hassle to convince oneself things work as intended).

@MaksimShakavin
Copy link

Hi @Treora, are there any new on this?

@Treora
Copy link
Owner

Treora commented Apr 20, 2021

Hi @Treora, are there any new on this?

No news; I have not touched this module in ages as I have not worked on any redux-based projects, and have too many other things to do. If it would be of help to people I could merge a fix or even add a feature, if it does not require much involvement from my side (ideally other users would also help with reviewing, though I am not sure if/how many people might be interested).

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.

3 participants