Fixes #21137 - Register reducers from plugins

[xprazak2]

This allows us to register reducers from plugins and hook into different levels of our state tree.

// foreman_plugin/webpack/index.js
import { injectReducer } from 'redux-injector';
import pluginState from './reducers';

injectReducer('hosts.foremanPlugin', pluginState);

State tree should be just a simple object (with uncombined reducers in leaves) if we want the states to be extensible. Using combineReducers will work as usual, but no one will be able to hook into the states that were combined manually.

[theforeman-bot]

Issues: #21137

[houndci-bot]

Missing semicolon semi

[ohadlevy]

/cc @waldenraines

[waldenraines]

@xprazak2 thanks, this looks good to me and worked for my test of creating a simple store. Was easy to use, nice approach.

[ohadlevy]

Indeed it does look nice, two concerns I have are:

1. We can't use ng-redux (if we want to start moving some of the current angular code to redux).
2. We can't customize the store to use local storage.

Maybe we should consider sending a pr for those as it's indeed a nice solution

[xprazak2]

I rebased, regarding the concerns:

1. ng-redux currently creates a new store, hooking into the existing one is on the roadmap. The fix will not be a one-liner, but perhaps we could try to move it along.

2. there are already multiple npm modules that connect store to (not just) local storage. Before moving forward with local storage, I think we need to decide our use cases, how often we want to save into local storage and whether whole store or just selected state subtrees should be saved.

[xprazak2]

Failing test hints at possible complications further down the road. I made a change that should turn the js tests green.

[danseethaler]

Any work on this recently? Seems like the builds have become more stable. Probably worth rebasing and testing again. We're going to need this feature is Katello asap.

[danseethaler]

Just saw the PRs submitted by @xprazak2. Looks like redux-injector has been abandoned. The code is very light and it seems like it would be worth just pulling into our code base. Thoughts?

[waldenraines]

Looks like redux-injector has been abandoned.

What makes you say that? I briefly perused the repo and didn't see anything about it being abandoned.

[xprazak2]

Current status: I rebased, but I expect tests using snapshots to fail. I submitted a patch to redux-injector 21 days ago that should take care of it, but my PR is without a response from maintainer. I did not see any mentions that the repo is abandoned/deprecated either, the oldest PR has been opened since February without any comments from maintainer. Possible ways to move this forward:

1. wait to get a response on proposed change and get it into redux-injector
2. fork redux-injector into theforeman
3. pull it directly into our codebase

[ares]

I vote for 2, it seems the project does not move anywhere since last release in 7 Nov 2016. I suppose we could call it tfm-redux-injector.

It does not specify license :-/

[danseethaler]

IMO there is so little code it's worth just pulling into our code base. I have an example that doesn't manipulate the store (or snapshots) and allows registering reducers async.

danseethaler@97a0158

If we did use this approach here's a gist of what the plugin implementation would look like https://gist.github.com/danseethaler/d745921bebbe5f696c4a316e38d8879f

I'm certainly up for whatever works and is simplest while also hoping to get something merged soon.

[danseethaler]

Just to clarify, I'm suggesting that we don't use redux-injector at all and instead just write a solution ourselves. The link to my branch in my last comment is an idea of how that would work. I've tested it out with Katello it works great.

I think this is the simplest approach and I don't see any downsides but I'm certainly open to other ideas and/or concerns.

[xprazak2]

The whole redux-injector is ~ 50 lines and I do not expect our own solution to be bigger than that, so having our own solution seems feasible. Plus, we would not have to introduce another dependency.

#5014 is the minimal working solution, so if @danseethaler is willing to reopen, we can get it in and move from there.

[danseethaler]

Thanks @xprazak2, I've reopened #5014 and I'm adding some comments there.

[xprazak2]

Closing in favour of #5014