Add basic Redux infra based on ng-redux #2504

vojtechszocs · 2017-10-24T16:07:51Z

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 · 2017-10-24T16:26:06Z

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 · 2017-10-25T06:49:05Z

/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 · 2017-10-25T06:56:18Z

app/assets/javascripts/application.js

@@ -14,6 +14,7 @@
 //= require angular-bootstrap/ui-bootstrap-tpls
 //= require angular-sanitize
 //= require angular.validators
+//= require ng-redux/dist/ng-redux


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.

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.

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 · 2017-10-25T06:57:05Z

app/javascript/miq-redux/middleware.ts

@@ -0,0 +1,5 @@
+import { Middleware } from 'redux';


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

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.)

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

ohadlevy · 2017-10-25T11:18:13Z

app/javascript/packs/miq-redux-common.ts

@@ -0,0 +1,24 @@
+import { IModule } from 'angular';


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

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 · 2017-10-27T13:16:56Z

package.json

@@ -23,11 +23,15 @@
    "@angular/platform-browser": "~4.0.3",
    "@angular/platform-browser-dynamic": "~4.0.3",
    "core-js": "~2.4.1",
+    "ng-redux": "^3.5.2",
+    "redux-devtools-extension": "^2.13.2",


No redux dependency?

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 · 2017-10-27T13:23:06Z

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 · 2017-11-01T16:59:07Z

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 · 2017-11-09T13:56:25Z

@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 · 2017-11-10T14:30:41Z

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

What that guy said!

vojtechszocs · 2017-11-15T11:42:02Z

@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 · 2017-11-15T14:06:38Z

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 · 2017-11-15T14:53:53Z

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

vojtechszocs · 2017-11-15T15:30:38Z

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 · 2017-11-15T19:09:49Z

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 · 2017-11-16T16:52:08Z

@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 · 2017-11-24T14:17:03Z

@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.

Signed-off-by: Vojtech Szocs <[email protected]>

https://github.com/rails/webpacker/blob/master/docs/folder-structure.md Signed-off-by: Vojtech Szocs <[email protected]>

Signed-off-by: Vojtech Szocs <[email protected]>

vojtechszocs · 2017-12-01T17:32:21Z

@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 · 2017-12-01T18:41:43Z

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 · 2017-12-01T18:43:26Z

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

ohadlevy · 2017-12-01T18:50:56Z

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

vojtechszocs · 2017-12-04T17:28:33Z

@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 · 2017-12-05T13:10:44Z

@martinpovolny @ohadlevy shall we merge this one?

martinpovolny · 2017-12-13T10:59:19Z

@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 · 2017-12-14T09:04:05Z

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

himdel · 2017-12-15T16:42:18Z

@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 · 2017-12-17T17:52:41Z

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

martinpovolny · 2017-12-23T10:25:49Z

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

oourfali · 2017-12-23T10:42:55Z

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 · 2017-12-23T15:28:38Z

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

miq-bot · 2017-12-23T15:34:05Z

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 · 2017-12-23T17:19:40Z

Thanks! On Dec 23, 2017 7:17 PM, "Martin Povolny" <[email protected]> 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> .

ohadlevy reviewed Oct 25, 2017

View reviewed changes

karelhala reviewed Oct 27, 2017

View reviewed changes

miq-bot added the enhancement label Oct 27, 2017

miq-bot assigned martinpovolny Oct 27, 2017

vojtechszocs mentioned this pull request Nov 1, 2017

[WIP] Add extensible components API to create components which can expose API and render endpoints #2060

Closed

2 tasks

vojtechszocs force-pushed the redux-infra branch from 3efe18b to 999b07d Compare November 15, 2017 14:30

vojtechszocs added 6 commits December 1, 2017 18:13

Add basic Redux infra based on ng-redux

53e00cc

Drop 'miqApp' webpack compile-time alias

1ac206b

Signed-off-by: Vojtech Szocs <[email protected]>

Adhere to Webpacker folder structure

6c928aa

https://github.com/rails/webpacker/blob/master/docs/folder-structure.md Signed-off-by: Vojtech Szocs <[email protected]>

Describe global structures for better type checking

abf968a

Signed-off-by: Vojtech Szocs <[email protected]>

Change import to require in pack/entry file

9ac7fdc

Signed-off-by: Vojtech Szocs <[email protected]>

Add .eslintrc.json for webpack'ed JavaScript

90ee5a0

Signed-off-by: Vojtech Szocs <[email protected]>

vojtechszocs force-pushed the redux-infra branch from 199c190 to 90ee5a0 Compare December 1, 2017 17:14

karelhala approved these changes Dec 6, 2017

View reviewed changes

miq-bot added the unmergeable label Dec 14, 2017

ohadlevy mentioned this pull request Dec 17, 2017

[POC/WIP] Redux integration #1921

Closed

Merge branch 'master' into redux-infra

c363266

miq-bot removed the unmergeable label Dec 23, 2017

martinpovolny merged commit fe5a549 into ManageIQ:master Dec 23, 2017

martinpovolny added this to the Sprint 76 Ending Jan 1, 2018 milestone Dec 23, 2017

martinpovolny added javascript infrastructure gaprindashvili/no developer labels Dec 23, 2017

himdel mentioned this pull request Jan 2, 2018

Fix ReferenceError: jasmine is not defined #3146

Merged

vojtechszocs mentioned this pull request Feb 9, 2018

Add new JS component API #3228

Closed

himdel mentioned this pull request Apr 18, 2018

Add direct redux npm dependency, redux 4.0.0 breaks #3790

Merged

Add basic Redux infra based on ng-redux #2504

Add basic Redux infra based on ng-redux #2504

Conversation

vojtechszocs commented Oct 24, 2017

Add basic Redux infra based on ng-redux

vojtechszocs commented Oct 24, 2017

ohadlevy commented Oct 25, 2017

ohadlevy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vojtechszocs Nov 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karelhala commented Oct 27, 2017

vojtechszocs commented Nov 1, 2017

himdel commented Nov 9, 2017 • edited Loading

martinpovolny commented Nov 10, 2017

vojtechszocs commented Nov 15, 2017

vojtechszocs commented Nov 15, 2017

vojtechszocs commented Nov 15, 2017

vojtechszocs commented Nov 15, 2017 • edited Loading

vojtechszocs commented Nov 15, 2017

vojtechszocs commented Nov 16, 2017

himdel commented Nov 24, 2017

vojtechszocs commented Dec 1, 2017

vojtechszocs commented Dec 1, 2017

vojtechszocs commented Dec 1, 2017 • edited Loading

ohadlevy commented Dec 1, 2017

vojtechszocs commented Dec 4, 2017

oourfali commented Dec 5, 2017

martinpovolny commented Dec 13, 2017

miq-bot commented Dec 14, 2017

himdel commented Dec 15, 2017 • edited Loading

ohadlevy commented Dec 17, 2017

martinpovolny commented Dec 23, 2017 • edited Loading

oourfali commented Dec 23, 2017 via email

martinpovolny commented Dec 23, 2017 • edited Loading

miq-bot commented Dec 23, 2017

oourfali commented Dec 23, 2017 via email

vojtechszocs Nov 1, 2017 •

edited

Loading

himdel commented Nov 9, 2017 •

edited

Loading

vojtechszocs commented Nov 15, 2017 •

edited

Loading

vojtechszocs commented Dec 1, 2017 •

edited

Loading

himdel commented Dec 15, 2017 •

edited

Loading

martinpovolny commented Dec 23, 2017 •

edited

Loading

martinpovolny commented Dec 23, 2017 •

edited

Loading