-
Notifications
You must be signed in to change notification settings - Fork 356
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
[POC/WIP] Angular component with redux settings #1944
[POC/WIP] Angular component with redux settings #1944
Conversation
app/javascript/miq-redux/index.ts
Outdated
@@ -0,0 +1,14 @@ | |||
import {createStore} from 'redux'; | |||
import {addReducer, applyReducerHash} from './reducer'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: inconsistent formatting/spacing, maybe consider using something like prettier or some other editor configuration :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or tslint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fon't know why, but for some reason my VS code did not picked up .editorconfig and went on it's own with formatting. But I will definetely use tslint to be mean to everyone if they'll make any mistake.
import { AppState } from '../packs/miq-redux'; | ||
|
||
const asdf = 'INIT_NEW_PROVIDER_HAWKULAR' | ||
function initNewProvider(state, action): AppState { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick - can be written as
const initNewProvider = (state, action): AppState => (
{...state, data: action.payload.data}
);
see https://egghead.io/lessons/javascript-redux-simplifying-the-arrow-functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or tassign
which is the typesafe version of Object.assign
so it will only allow valid properties to be used.
https://www.npmjs.com/package/tassign
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally like a lot the spread operator, but in this case I was mostly referring to arrow functions vs functions (that require return etc)
app/javascript/miq-redux/reducer.ts
Outdated
return newState; | ||
}; | ||
|
||
export function addReducer(reducer: AppReducer) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: inconsistent, once using a function another time es6 :)
app/javascript/miq-redux/reducer.ts
Outdated
let newState = state; | ||
|
||
reducers.forEach((reducer) => { | ||
newState = reducer(newState, action) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand this, why not use combineReducers which just accepts all reducers?
I don't think you are required to loop over all reducers yourself?
Also, I can see nested reducers (e.g. many levels deep).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We basically try to aim for simplicity and flexibility.
The addReducer
(miq-redux) function is to be used by both core (manageiq-ui-classic) and plugins. You start with no reducers (empty Set
) and allow client code to add new ones.
In a component-oriented architecture with Redux, you have specific components bound to (or data-driven by) specific parts of the state tree. Having one reducer per one logical component seems like a natural fit for such architecture.
As you wrote, one would normally use Redux combineReducers
helper to combine smaller reducers into bigger ones. The difference here is that we don't (and shouldn't) know in advance what the specific reducers will be. We simply provide a way to add your reducer via addReducer
. This means the MiQ Redux infra is opt-in by design, allowing gradual adoption of Redux architecture, typically on per-component basis.
Calling all reducers in a forEach
loop reflects the design that "all reducers are on the same level" - they all get the whole state, the action that was dispatched, and impose changes on the relevant part of the state tree (while following the general principle of state immutability). This design follows the goals mentioned above - simplicity and flexibility.
We could, by design, constrain each reducer to a specific state subtree. It would probably result in API like addReducer(myReducer, 'foo.bar')
where myReducer
would be constrained to operate only on a subtree represented by the expression state.foo.bar
. But do we really need that?
All reducers, both core and plugin contributed, are to be treated in the same way from MiQ Redux infra perspective. If a reducer does something harmful, like overwriting a specific state subtree in an inconsistent way, it's a problem with that particular reducer (or generally with the code that added such reducer).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the detailed answer. I don't see the two conflict. E.g. we should have an API to add redcuers (or a whole bunch of reducers) but then pass it (array of reducer functions) as argument to combine reducers which will do the actual looping and basic tests (return values etc).
This will easily allow you to create sub reducers naturally (which I can't are us going without)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vojtechszocs what do you think about this pattern instead? https://medium.com/@SntsDev/the-factory-pattern-in-js-es6-78f0afad17e9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ohadlevy thanks for the link. I'd prefer the API to be as simple as possible 😃 I guess we should give it some more thought.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vojtechszocs I don't think that changes the API rather how its implemented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it depends on how the API would look like. Right now, there's a simple addReducer
function. Can you share some code snippet of how you'd imagine the reducer composition in MiQ Redux infra?
app/javascript/miq-redux/store.ts
Outdated
|
||
import {AppState} from '../packs/miq-redux'; | ||
|
||
const composeEnhancers = window['.__REDUX_DEVTOOLS_EXTENSION_COMPOSE__'] || compose; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra dot before __REDUX ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good one. Did not used the browser's febugger, yet. But I'd definetely wonder why it's not working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should be
window.__REDUX_DEVTOOLS_EXTENSION__ && window.__REDUX_DEVTOOLS_EXTENSION__(),
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We were planning ahead and added support for middlewares as well, and based on their docs we should use advanced-store-setup with middlewares.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are correct, I didnt see that example, in general its not required if you don't provide an initialState but that make sense long term too (caching in session storage etc).
just a question, why do you access it via [] vs
const composeEnhancers = window.__REDUX_DEVTOOLS_EXTENSION_COMPOSE__ || compose;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because typescript is sometimes too aggressive and was complaining that __REDUX_DEVTOOLS_EXTENSION_COMPOSE__
does not exists on window.
But since you brought it up, which one would you prefer? Because I am Indecisive between accessing it via [] or re-typing window object to any.
const composeEnhancers = (<any>window).__REDUX_DEVTOOLS_EXTENSION_COMPOSE__ || compose;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the __REDUX_DEVTOOLS_EXTENSION_COMPOSE__
global is well-known in advance, I'd prefer the following form:
window.__REDUX_DEVTOOLS_EXTENSION_COMPOSE__
and tell TypeScript to accept (recognize) that global. I'd use the []
access mainly for dynamic properties.
Since Redux compose
returns a function, the TypeScript-annotated property access should be (<function>window)
.
Anyway, I think the proper solution should be to declare this global using namespace so that one doesn't have to always use (<function>window)
access prefix.
Before removing wip, please make sure to use 2 spaces indent consistently, not 4, not 8 :). |
@himdel how would you feel we add hound support for js and css? |
@himdel this PR will proably be not part of ui-classic, but those 4 spaces bugs me as well. Btw what do you and @martinpovolny think about moving this to a separate repo which will be rails plugin and also npm package? |
@ohadlevy Hound probably doesn't bring us anything (or does it?).. we're already using codeclimate (and a bunch of others).. I think the problem is that the existing EDIT: I guess what I'm saying is that CI support is secondary to figuring out the right way to lint TS first :) |
@himdel that's quite easy to do, we should use eslint-loader, plus adding |
Adding eslintrc.json to app/javascript is clear, yeah, we were using rules based on So the es6 version would be probably based off As for |
.. But tested that current eslint can actually handle .ts code just fine :) So.. doing a PR to add that eslintrc.json and we'll see what happens :) |
Hi everyone, here are some thoughts after reading the recent comments. The initial I'm all for reducer composition given that its implementation for our Redux infra is simple enough. If you look at Redux
Proposed MiQ Redux infra allows dynamic addition of reducers, therefore the "reducers are known in advance" doesn't hold, which in turn complicates change detection and reducer composition in general. In other words, MiQ Redux infra is not a typical Redux application (not SPA, not fully componentized and therefore not fully Redux-aware). I agree with @ohadlevy about using
If you plan to test your Angular components, it's a good idea to separate the "connect to Redux" concern by writing a higher-order (also known as "connected") component. |
Why can't we assume the reducers are registered at some "app startup time" e.g. I can't see a reducer being added dynamically (just like a plugin won't be added on the fly) ? In that case, IMHO we should be able to know the reducers in advance (and that's why I suggested the registration / factory pattern).
👍 this will make our angular / react implementations to be fully disconnected from the store structure, allowing us to refactor in the future, or in other words, angular code should never talk to the store regardless. |
As for reducer composition, I gave it some more thought. Redux Therefore, we can come up with some API that allows reducer composition (effectively scoping reducers to specific state subtrees) while not worrying too much about state change detection. All listeners attached to MiQ Redux store should use |
I'd prefer not to make such assumption. Lazy loading (code splitting) reducers is a valid use case. All JS packs (core and plugin contributed) execute after core MiQ JS code. By defining "app startup time" we remove some flexibility. |
Is it? TBH I can't see that ?
On the other hand we embrace a journey that is different than many other apps using this technology, so before we go on that path, we are doing it because we have the correct requirements? |
@ohadlevy we can't see yet how MiQ Redux infra will be used, we're just designing it now and understanding our requirements, based on components we're developing (new middleware form - in progress). I'd suggest to keep the most simple and flexible design until we have a couple of Angular components connected to Redux store, extended using e.g. React technology and having those extensions also connected to Redux store as well. With that done, we will have a better picture of what we need & how we actually use the Redux infra. I've also discovered an interesting project called extensible-duck which defines a standard way to bundle reducer, action types & action creators in a single logical package, aka the "duck". We might end up doing something similar, but at this point, I'd keep things as simple as possible. What drives our requirements are (will be) the Angular components and their extensions, I think. |
d4431c4
to
70c8b1c
Compare
I take it as that the redux part is solid and can be used like this, am I correct? If so, we should move redux related stuff to it's own repo which will have npm package settings and it will be ruby plugin based on https://github.com/martinpovolny/miq_plugin_example. However this can be done later on. PR definition has been updated to correspond current state. |
@vojtechszocs take a look at app/javascript/extensible-components/index.ts and app/javascript/extensible-components/lib.ts. It's partly the implementation of extensible component which we discussed together. |
@karelhala - I did some code improvements on top of your commits, see here and here. I didn't push them into this PR yet, let me know what you think.
I think the MiQ Redux part is simple and solid enough for us to proceed further. I see several possible improvements, but these can be done later on. Some ideas for the future: 1, Allow reducers to be scoped to a particular state subtree. For example, all reducers defined in
2, As suggested by @ohadlevy - draw inspiration from React world: whenever Redux state changes, map state object into necessary bits of data (similar to react-redux mapStateToProps function) and re-render the given component. In this PR, we use Angular v1
But this is due to using Angular v1. In Angular latest, there should be no local (component-specific) state. Additionally, we should use libraries like reselect to compute any derived data, as well as memoizing any state projections for better performance.
Yes, that's the future. I'd recommend to keep hacking this PR a bit more to cover component extension & reusability as well. From MiQ core perspective, it will be a Rails plugin that contributes all the infra & APIs through Webpacker's From MiQ (JavaScript) plugin perspective, it will be a monorepo (repo hosting multiple npm packages):
We can consider using tools like Lerna to manage the monorepo.
Looks good to me! If I had to generalize the Redux workflow for form-based components, I'd say that:
Given that condition, we can have a single pair of I'm going to review the latest commit that addresses component extension. PS: sorry for my late response, I was out during the recent days. |
…PI and render endpoints
… extensible component
… extensible component
By Karel Hala and Vojtech Szocs.
…vider object has changed
…dren to redux store and unbind them when neccessary Add changeForm method to default form Controller Init form from default form controller Add JS doc comments to defaultForm
89417a1
to
64fa5dd
Compare
Don't get scared that there are extensible component commits as well. This PR relies on it, so it's rebased on top of it. Once #2060 will be merged these commits will not be here. |
64fa5dd
to
9195622
Compare
Checked commits karelhala/manageiq-ui-classic@6d7d3e2~...c62816c with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 app/views/ems_middleware/new.html.haml |
Adding link to redux-form which is React-based, but we can still draw inspiration from it. It's basically a library to connect existing form components to a Redux store. See it in action here. The fundamental difference between
For Angular 1.x form component impl. like in this PR, we can consider doing (2) above, while also exploring the possibility of having field-specific lifecycle hooks like format, parse, normalize etc. Therefore, I'd suggest to split this PR into two commits, one for the Redux infra itself, and one for Angular-specific helpers that connect Angular components to a Redux store. |
This pull request is not mergeable. Please rebase and repush. |
@karelhala, @vojtechszocs : what is the status of this PR? |
This pull request has been automatically closed because it has not been updated for at least 6 months. Feel free to reopen this pull request if these changes are still valid. Thank you for all your contributions! |
This PR introduces basic settings of redux in MiQ repository. In future redux will be in separate repository which will be rails plugin and also available as NPM package, so other plugins can include such package.
Working together with @vojtechszocs
Work very much in progress, so do not merge this.
EDIT:
Part of this PR is redux related stuff and some of it is using it from angular when creating basic form. This PR will later down the line drop redux stuff, which will be moved to it's own repository. However using redux from angular needs some explanation.
Using redux in angular component written in TS.
This abstract class will bind it's child class to redux store, if you'll pass any reducers hash it will apply them to rootReducer.
UPDATE_FORM
action and assign payload to correct place in state tree.INIT_FORM
action, which is fired inside$onInit
.updateFormObject
method, which is fired every time state changes.formObject
as seen in updateFormObject method.onChangeForm
function usingng-change
Default actions
Since there can be only one form per page and since when registering to rootReducer we provide callback to delete this reducer from rootReducer we can have couple of default actions which can forms use.
onChangeForm
which will passformObject
as payload.Reducers hash
To ease creation of reducers we introduced somewhat different way of creating reducers, you don't have to write complex switch statement, which can be hard to read and big and complex reducers can lead to slower state propagation. You will specify object with actions as keys and values will be function or function callbacks. For example take a look at simple reducer with one function callback and one inline function.
Extensible components
If some component decides that it would like to be extended by some plugin, it can use new feature called extensible component. It sits under
ManageIQ.extensionComponents
where you have 2 things:So to register some component to be extensible call
And if some plugin wants to extend such component, just call (inside plugin)
ManageIQ.extensionComponents.source.subscribe()
with some inner logic.