Add basic Redux infra based on ng-redux

[vojtechszocs]

Add basic Redux infra based on ng-redux

This PR adds new namespace, ManageIQ.redux, that can be used by both Rails-managed (ES5) and webpack(er)-managed (ES6+) frontend code.

Redux store implementation is based on ng-redux for easy integration with existing Angular 1.x code through the $ngRedux.connect API. Note that the $ngRedux object exposes all of the standard Redux store APIs like dispatch and subscribe, in addition to custom APIs like connect. (Behind the scenes, ng-redux uses standard Redux createStore and augments it with additional methods.)

Proposed ManageIQ Redux API is minimal, here's the TypeScript interface:

interface ReduxApi {
  // access to Redux store, returns the $ngRedux object
  store: ReduxStore;
  // MIQ Redux core API, allows adding new reducers
  addReducer(appReducer: AppReducer): Unsubscribe;
  // MIQ Redux helpers, see the JSDoc for details
  applyReducerHash(reducerHash: AppReducerHash, state: AppState, action: Action): AppState;
}

The Redux infra is represented as a new "common" pack, app/javascript/packs/miq-redux-common.ts, which makes it available to frontend code across all web pages (e.g. Rails views).

There are currently no Redux middlewares applied, but we should consider using redux-observable (or similar) for handling async operations and side effects from within all registered reducers.

There's also support for Redux DevTools browser extension for better inspection of UI state.

All Redux-specific TypeScript declarations are in app/javascript/packs/custom-typings.ts. To make tsc accept known globals, I've added a separate (utility) pack, app/javascript/packs/custom-typings.ts.

Jasmine tests include:

• availability of ManageIQ.redux namespace
• availability of standard Redux methods on the $ngRedux object
• Redux typical workflow compliance using the $ngRedux object
• unit tests for individual components like rootReducer, addReducer etc.

Things to consider as follow-ups:

• using appropriate Redux middleware(s) to extend capabilities of individual reducers
• initialState persistence across page reloads

[vojtechszocs]

Suggestions for future improvements:

• current Jasmine version 2.5.2 is very old, consider bumping it up
• even better, consider replacing (gem-provided) Jasmine with (npm-provided) Jest ⭐️, applied on both Rails-managed and webpack(er) managed code, with ability to write new tests using ES6 and TypeScript (⭐️ or any testing tool that does things like ES6-to-ES5 transpilation for you, removing complexity from your existing test infra)
• I've noticed that yarn.lock is .gitignore'd which is generally not recommended, as it results in potentially different dependency trees across different environments
• in the long term, consider having all frontend dependencies managed through a single channel, currently it's package.json + bower.json + frontend stuff in manageiq-ui-classic.gemspec

Next up, I'll start working on adding basic extensible components API based on the existing proposal.

[ohadlevy]

/cc @chalettu

[ohadlevy]

thanks @vojtechszocs I've left a few inline comments, overall looking good from reading mostly the tests, I still find it much harder to read typescript.

[ohadlevy]

why do we need to load it via the asset pipeline? I was under the assumption we can load it directly to the browser from webpack.

[karelhala]

Well this is because the main angular app is bootstrapped here miq_angular_application.js#L1 and sadly you cannot add dependencies (modules), once app is loaded.

[vojtechszocs]

We need to run ng-redux main script which registers the ngRedux module and hook it to the Angular application module.

In miq_angular_application.js we declare ngRedux module as a dependency of ManageIQ (application) module. The important bit is that miq_angular_application.js is Rails-managed.

My understanding is that when Rails-managed miq_angular_application.js executes, the ngRedux module registration must be already done - and this implies that we need to load ng-redux library through application.js file.

you cannot add dependencies (modules), once app is loaded.

Officially yes, unofficially there's a way but it only works if you add those extra dependencies prior to application bootstrap (there are some undocumented APIs used by Angular injector).

[ohadlevy]

i find the name a bit confusing with the middleware provider, maybe we should comment that this is redux middleware :)

[vojtechszocs]

I can rename this file to redux-middleware.ts to reduce confusion.

(Still, this file is part of miq-redux pack which implicitly narrows down the context to Redux.)

[martinpovolny]

yes, the path javascript/miq-redux is pretty clear

[ohadlevy]

should we extract angular specifics into a different pack / file? just wondering if we move away from angular, ideally redux core implementation should not change

[vojtechszocs]

Following files depend on Angular specifics (1.x module API):

• ng-redux based Redux store configuration: miq-redux/store.ts
• pack entry point: packs/miq-redux-common.ts
• test helper (to allow testing specific module exports): miq-redux/test-helper.ts

If we'd decide to move away from Angular, we'd have to change store creation, entry point code as well as tests (which currently use inject to accept Angular managed objects).

However, I could extract all Angular stuff as shown above into a single file, say miq-redux/angular-config.ts.

[karelhala]

No redux dependency?

[vojtechszocs]

No redux dependency?

redux is pulled in through ng-redux as transitive dependency, see here.

ng-redux@^3.5.2 relies on redux@^3.7.2 and let's say we add dependency on redux@^4.x this might break ng-redux expecting version 3.x APIs. This is why I didn't include redux dependency here.

[karelhala]

Looks good! I like that you stayed strictly to types and even used some derived types. This will definitely helps us in future, when someone will use this from some clever IDE which can make suggestions based on types.

This might be interesting for you @himdel, @skateman, @dclarizio, @h-kataria, @AparnaKarve, @martinpovolny.

@miq-bot add_label enhancement
@miq-bot assign @martinpovolny

[vojtechszocs]

overall looking good from reading mostly the tests, I still find it much harder to read typescript.

Well, I could change the code to use Flow type annotations along with babel-preset-flow applied to babel-loader 😃 but TypeScript isn't that much different.

The main difference for me is:

• in TypeScript, some ES6 language constructs like classes are re-implemented (conceptually the same, but possibly a slight difference in syntax)
• in TypeScript, there are some new language constructs like interfaces, namespaces etc.

If TypeScript was used only for type checking, above mentioned difference would be irrelevant, but as of today, tsc runs both type checking and transpilation (unlike Flow, which is meant for type checking exclusively).

Note that current webpack(er) configuration supports both TypeScript and regular ES6, possibly with Flow-annotated code (using babel-preset-flow), to co-exist together under app/javascript. But it doesn't make much sense to mix two different type checkers in the same code base, so this is more about the vision of where do we want to converge to.

Looks good! I like that you stayed strictly to types and even used some derived types. This will definitely helps us in future, when someone will use this from some clever IDE which can make suggestions based on types.

Thanks!

Question: this PR currently adds app/javascript/packs/custom-typings.ts which webpacker sees as a regular pack, so it gets compiled into manageiq/public/packs/manageiq-ui-classic/custom-typings.js. I'm not a TypeScript expert so I'm wondering if there isn't a better way to handle this.

[himdel]

@vojtechszocs Would you mind dropping that miqApp alias please?

The moment we start using a different name for the ManageIQ global somewhere is the moment we can no longer track where it's being used by that name.

EDIT: if this was so that you don't have to type out (<any> window). every time, I'm fine with

+    new webpack.DefinePlugin({
+      ManageIQ: 'ManageIQ'
+    }),

[martinpovolny]

... moment we can no longer track where it's being used by that name.

What that guy said!

[vojtechszocs]

@himdel I've added that miqApp alias to save me from typing ManageIQ.angular.app but I see your point and I agree with you, I'll drop that.

[vojtechszocs]

Note that I'd really love to do things like

import { ReduxApi } from '../miq-redux/redux-typings';

inside app/javascript/packs/custom-typings.ts and consequently improve type definition of the ManageIQ global. But when I add such import statement, that whole file seems to be ignored during compilation and webpack fails on stuff like Cannot find name 'ManageIQ'. I don't know how to resolve this issue for now, perhaps some Webpacker/TypeScript guru can tell me what's the best way to define common typings across packs, while allowing those typings to import stuff from other places.

[vojtechszocs]

I've just read about Webpacker folder structure, will do some more changes.

[vojtechszocs]

Solved the issue around d.ts files failing when they contain import statements, stuff from test-helper.ts moved into pack's entry.

Global variables such as ManageIQ have their structure (interface) properly described for better type checking.

[vojtechszocs]

All done and tested locally, please review 💻

The redundant custom-typings pack is no more; its declarations were moved to app/javascript/typings/globals.d.ts. This means that manageiq/public/packs/manageiq-ui-classic now contains only one additional output file, miq-redux-common.js, as it should.

Solved the issue around d.ts files failing when they contain import statements

Solution based on this comment, kudos to @geekflyer!

@himdel @karelhala with TypeScript global declarations in globals.d.ts, writing

const app = ManageIQ.angular.app;

is subject to type checking and automatically infers app variable's type.

[vojtechszocs]

@himdel @karelhala CC/ESLint reports that 'require' is not defined.

Should I update globals in .eslintrc.json? Or would you prefer me adding app/javascript/.eslintrc.json to refine the base config?

[himdel]

@vojtechszocs The latter please - we're already overriding the config for app/assets/javascript/{controllers,components,directives,services}/ to enable angular-related rules, so let's do the same thing for app/javascript.

I'm guessing we should mostly stick to airbnb-es6, given we've built our es5 rules on top of their es5 ruleset.

[vojtechszocs]

@himdel @karelhala PR updated, added .eslintrc.json for webpack'ed JavaScript.

For now, the .eslintrc.json contains:

{
  "globals": {
    "require": false
  }
}

Why? I've noticed that package.json already has dependency on eslint-plugin-standard and eslint-config-standard which represent the Standard JS code style. Personally, I prefer Standard JS over other code styles, but I know that Airbnb is quite popular.

I think there should be another PR that makes the decision whether to use Standard JS or Airbnb (or whatever) and reflects that decision within .eslintrc.json for webpack'ed JavaScript.

Nit-pick: I see some duplicity across ESLint configs at app/assets/javascript/{controllers,components,directives,services} given that Rails'ed JavaScript is generally Angular-based.

Running yarn run eslint app/javascript locally gives no errors, so I'd say this PR is complete now?

[vojtechszocs]

jasmine server started

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.

How can I retrigger Travis if I don't have write access to the repo?

[vojtechszocs]

@oourfali Once this PR is merged, component extension & reuse PR is planned as the next step.

[ohadlevy]

@vojtechszocs i was able to restart it by clicking the restart btn on travis, not sure which permissions are needed for that.

[vojtechszocs]

@vojtechszocs i was able to restart it by clicking the restart btn on travis, not sure which permissions are needed for that.

There's an open issue on Travis CI to allow commit authors to restart builds, however without any recent activity.

Looks like you need to have repo write access (e.g. be a maintainer) in order to see the restart button on Travis CI build page. A common workaround is to close PR and re-open it again, but this adds noise to the PR activity.

Maybe the ManageIQ bot could handle this via some @miq-bot travis_restart <travis_build_id> using Travis CI API e.g. POST /builds/{build.id}/restart ?

[oourfali]

@martinpovolny @ohadlevy shall we merge this one?

[martinpovolny]

@himdel: is there any remaining reason NOT to merge this yet?

I see none, but I'd love to get your explicit ACK ;-)

[miq-bot]

This pull request is not mergeable. Please rebase and repush.

[himdel]

@martinpovolny No reason I'm aware of :), I'm not seeing any obvious breakage, the code looks like it is not intrusive enough to give use cause to hold merging this... everything's fine 👍

(The only reason I've not merged this myself is that I haven't had the time to go through the code and properly understand this. Feel free to do so if you do.)

[ohadlevy]

as it stands, this is better merged than not, 👍 to merge.
@vojtechszocs sadly a rebase is needed...

[martinpovolny]

@vojtechszocs : Gaprindashvili almost out of the door. Please, rebase, so that we can merge this. Thx!

[oourfali]

Vojtech is sick. Can someone else help rebasing?

…

On Dec 23, 2017 12:25, "Martin Povolny" ***@***.***> wrote: @vojtechszocs <https://github.com/vojtechszocs> : Gaprindashvili almost out of the door. Please, rebases, so that we can merge this. Thx! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2504 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAyzAQ2CynpZDw3zsvPVrB4JP3o3LJzqks5tDNUtgaJpZM4QEsBN> .

[martinpovolny]

Sure. Done. Let's see what travis thinks about it.

[miq-bot]

Checked commits vojtechszocs/manageiq-ui-classic@53e00cc~...c363266 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. 🏆

[oourfali]

Thanks! On Dec 23, 2017 7:17 PM, "Martin Povolny" <notifications@github.com> wrote: Merged #2504 <#2504>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2504 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAyzAaYM8jy88i9-q9CWOg11bhZLmwxeks5tDTWIgaJpZM4QEsBN> .