Introduce redux state management for react portion of dashboard #14199

stacey-gammon · 2017-09-27T19:14:46Z

Introduces Redux state management for the react portion of dashboard, the "dashboard viewport". The top portion of dashboard - nav panels, filter bar, query, etc, are still all in angular. Eventually the goal is to replace all of those portions with react as well, and expand the state stored in redux.

Because there is still a large portion stored in angular, and dashboard currently uses AppState to manage that, as well as handle url changes, the DashboardStateManager handles keeping the redux store and the angular AppState in sync.

Unfortunately this does diminish the benefit of redux’s “single source of truth”. I think this can be handled once more of dashboard is in react, and with the introduction of react routing to handle URL changes. I decided for a first step, this was already a large PR, so did not want to take it any further.

I think this gives us a good way forward and provides benefits of clear state management, at least from the viewport downward.

Here is a diagram of how the new redux/angular state management interacts. It's still messy but at least the interaction is explainable.

stacey-gammon · 2017-09-28T01:53:11Z

src/core_plugins/kibana/public/dashboard/dashboard_state_manager.js

@@ -376,41 +435,6 @@ export class DashboardState {
  }

  /**
-   * Saves the dashboard.


Refactored this function to a separate file.

stacey-gammon · 2017-09-28T01:53:26Z

src/core_plugins/kibana/public/dashboard/dashboard_state_manager.js

 */
-function convertTimeToString(time) {


Refactored these functions to FilterUtils

stacey-gammon · 2017-09-28T01:53:55Z

src/core_plugins/kibana/public/dashboard/dashboard_app.js

 });

-uiRoutes


Moved this routing stuff to index.js to go with the other couple routes.

stacey-gammon · 2017-09-28T01:56:56Z

src/core_plugins/kibana/public/store.js

@@ -0,0 +1,12 @@
+import { createStore, applyMiddleware } from 'redux';


Because all kibana core apps are really part of a single plugin app, I created the store in a place that will be available to all of them, not just dashboard specific.

I'm not really for or against this idea, but is that the plan then? Each "app" will get one universal store?

Also, any consider for a more granular state layout? Having just a single top-level collection of reducers could result in gigantic and possibly hard to understand and work with collection of reducers. Maybe the intention is that we simply break things up as they grow?

I'm not really for or against this idea, but is that the plan then? Each "app" will get one universal store?

It was my idea, but I'm open to others as well. My reasoning is that with redux there is a single store, but each plugin is separate and so it will also have it's own redux store (or not, if it's in angular, or something else). Then they will all communicate via the new platform/build system (eventually).

Also, any consider for a more granular state layout? Having just a single top-level collection of reducers could result in gigantic and possibly hard to understand and work with collection of reducers. Maybe the intention is that we simply break things up as they grow

I was thinking it'd look like:

export const store = createStore( reducers, { dashboard: getInitialDashboardState(), visualization: getInitialVisualizationState(), discover: getInitialDiscoverState(), }, applyMiddleware(thunkMiddleware) );

But I could actually see some of that being taken out and placed globally - e.g. filters and the time picker which is relevant to all the core plugins.

All of this is flexible though. Doing it this way gives other apps the ability to add separate state info into the store, but also lets us re-organize later if need be.

I think time picker and other state all still fits this pretty well honestly. Especially if we use selectors, then the shape kind of doesn't matter.

My concern is more about the size of reducers I guess. Even considering just Dashboard, there could be a lot of actions and reducers there, and putting everything in a single file starts to get pretty hard to deal with. It's all just javascript, so we could push the concerns of breaking it down further into each of these app-level reducers, or we could just tweak stuff as we go in this root reducer definition.

Either way is fine, and starting off the way you have it is fine too, just curious what this might look like as things grow.

Ah yea, I think I'll end up breaking up the reducers into separate files. For right now, they seemed small enough where only one really has any logic, that putting them in a file was sufficient. But as the angular lessens and react grows, I imagine it will make more sense to split it out.

I think I might actually break some of it up now, as I'm switching to using the handleActions and createAction and following some of the patterns here https://github.com/bmcconaghy/x-pack-kibana/tree/license_management/plugins/license_management/public/store

stacey-gammon · 2017-10-02T15:56:13Z

cc @kjbekkelund, @w33ble, @chrisronline, @BigFunger, @bmcconaghy

You have all been working with redux implementations, so I wanted to ping you all and try to get some opinions on this attempt at introducing redux into a portion of the dashboard app. If you are busy, don't worry about it, just want to learn from others using redux and hopefully make sure we are all on somewhat of the same path forward.

stacey-gammon · 2017-10-02T15:58:00Z

Jenkins, test this

bmcconaghy · 2017-10-02T16:02:37Z

src/core_plugins/kibana/public/dashboard/dashboard_actions.js

@@ -0,0 +1,121 @@
+import {
+  EMBEDDABLE_RENDER_FINISHED,


I would look at redux-actions. Makes this all more concise: https://www.npmjs.com/package/redux-actions

I really dislike the action types pattern in the redux world.

To expand on Bill's recommendation, using redux-actions allows you to export and use your actions directly in both the reducers and in the containers, so the definition lives directly where it gets used and can be referenced all over the same way without the need for a separate collection of action types.

Most of these become something like

export const EMBEDDABLE_RENDER_REQUESTED = createAction('EMBEDDABLE_RENDER_REQUESTED');

bmcconaghy · 2017-10-02T16:03:52Z

src/core_plugins/kibana/public/dashboard/dashboard_reducers.js

+
+export const dashboardReducers = (state = getInitialState(), action) => {
+  switch (action.type) {
+    case EMBEDDABLE_RENDER_FINISHED: {


same as above re: redux-actions

w33ble · 2017-10-02T16:32:55Z

src/core_plugins/kibana/public/dashboard/dashboard_actions.js

+function embeddableRenderRequested(panelId) {
+  return {
+    type: EMBEDDABLE_RENDER_REQUESTED,
+    panelId: panelId


At the very least I think we should be using Flux Standard Actions and not just some free-form action shape (read the readme, the library isn't a requirement).

The short version is that action objects only every have a type, payload, conditional error, and optional meta property.

return { type: EMBEDDABLE_RENDER_REQUESTED, payload: { panelId: panelId } }

w33ble · 2017-10-02T16:41:01Z

src/core_plugins/kibana/public/dashboard/viewport/dashboard_viewport_provider.js

+import { Provider } from 'react-redux';
+import { DashboardViewportContainer } from './dashboard_viewport_container';
+
+export function DashboardViewportProvider(props) {


This is a total non-issue, and you certainly don't have to change this, but you can avoid the added return by defining simple SFC's as arrow functions. Just a quick tip.

export const DashboardViewportProvider = props => ( <Provider store={store}> <DashboardViewportContainer {...props} /> </Provider> );

w33ble · 2017-10-02T16:51:12Z

Looking at the root of the dashboard path, there's a lot of stuff in there:

I don't know that there's much, or possibly anything, to be gained by collecting concerns into other paths (like all the action, state, reducer stuff in a state path, for example), but I'm curious if you've considered it, and also what folks think about doing that. Certainly not a blocker here, file structure can be tricky so I like hearing other perspectives.

stacey-gammon · 2017-10-02T20:50:40Z

Looking at the root of the dashboard path, there's a lot of stuff in there:

@w33ble, definitely something I think needs improvement (though I don't think it should be part of this PR). I'm not really sure which folder structure is best. @kjbekkelund - has the platform team tackled this question at all? I'd be happy to follow suit if there has already been a "best practice" put in place. Otherwise, I would be interested in more follow up discussion regarding this.

kimjoar

Some feedback. Sorry, didn't have a lot of time for a proper review today, so some of the comments might "be a bit off" as I didn't have time to dig properly into the code.

Even though there's a few comments, I'm fine with merging as-is. I don't think any of them are big problems.

kimjoar · 2017-10-13T17:29:00Z

src/core_plugins/kibana/public/dashboard/reducers/panel.test.js

+import { getPanel } from '../reducers';
+import { getDashboardState } from '../../reducers';
+
+describe('panel reducers', () => {


Nitpick: I prefer removing the top-level describes (Jest already renders the file name itself in the tests, so it's already obvious)

kimjoar · 2017-10-13T17:36:41Z

src/core_plugins/kibana/public/dashboard/viewport/dashboard_viewport_container.js

+  } else {
+    return {
+      panelCount: Object.keys(getPanels(dashboardState)).length
+    };


This container "feels weird" as it's returning entirely different props (Sorry, I didn't have time today to explore ideas for changing it, just mentioning it, in case you or others have ideas). My "immediate idea" is that this should have been an if outside the component or that some intermediate component should be introduced. I found it a bit difficult to follow the flow because of this.

This ended up working out naturally when I made the expanded panel a purely css change and the grid is still all rendered.

kimjoar · 2017-10-13T17:41:34Z

src/ui/public/state_management/state_monitor_factory.js

-    changeHandlers.forEach(changeHandler => {
-      changeHandler(status, type, keys);
-    });
+    if (changeHandlers) {


This means that dispatchChange was called after destroy was called. It feels like that shouldn't happen, but maybe it's okey?

Without this change, an error is thrown when deleting a panel.

I can resolve the issue another way - by swapping the order of dispatches in onDeletePanel to first destroyEmbeddable and then deletePanel, but this feels like a fragile fix. Really the order shouldn't matter. Here is the issue with deleting the panel first:

DeletePanel is dispatched

This triggers _handleStoreChanges to be called inside of dashboard_state_manager.

This updates the panels array and calls saveState so appState updates the url accordingly.

saveState emits save_with_changes which the state monitor is listening to.

next destroyEmbeddale is dispatched.

this destroys the visualization DOM node, which in turn destroys a state monitor that was listening to save_with_changes

The next event loop starts and save_with_changes is triggered, which causes this to be called, but because destroy was already called, it throws an error.

If destroyEmbeddable is called first, this destroys the dom node and turns off the state monitor listeners before this.saveState emits the change event.

But the real problem is not that the order is wrong, but that off'ing an event emission won't stop that state monitor from reacting to already emitted events. Here's some debug output to show the problem: (I gave state monitor instances ids to track them)

... I actually went ahead and filed #14462. It probably won't be too difficult to fix the root problem and then I can remove this extra check.

Good catch!

kimjoar · 2017-10-13T17:45:56Z

src/core_plugins/kibana/public/dashboard/panel/dashboard_panel_container.js

+  getEmbeddableError,
+} from '../reducers';
+
+const mapStateToProps = ({ dashboardState }, { panelId }) => {


What I've always done with the selectors is using state here, not something inside state. So I've always ended up with something like getEmbeddable(state). I'm open to either approach. My argument for using state is that way we "never" walk down the state within the container, but as long as we're strict on only top-level I expect it won't be a problem.

However, my approach ended up creating https://github.com/elastic/cloud/blob/master/cloud-ui/public/reducers/index.js. I like that file, but I see the points from people arguing it's not DRY or that the file becomes too big, so it's not necessarily a good idea.

Yea, I worry if I put all selectors at the root of the core kibana store it would become a huge file over time (especially because it would include state for all the core apps, Visualize, Discover, Dashboard, .... etc). I'm also not just sure how I feel about all that knowledge about what exists below the state tree in the upper most container.

I could pass state and then change kibana/public/dashboard/reducers/index.js to wrap all the state passed into all those selectors in getDashboard from kibana/public/reducers.js, but then I'd be breaking the contract I currently abide by where each reducer gets state at it's own level.

I'm not really sure what the best approach is here. I think I'll leave as is for now and wait for more discussions before converting.

kimjoar · 2017-10-13T17:47:14Z

src/core_plugins/kibana/public/reducers.js

+ * core kibana plugins.
+ */
+export const reducers = combineReducers({
+  dashboardState


nitpick: I'd prefer just dashboard. It's already in the store, so I don't think appending state to all the keys are needed.

yea, I don't like the name dashboardState either. dashboard might get a little weird when we include more state like dashboard listing data. Then it'd be dashboard.dashboards or dashboard.currentDashboardId. Not sure what a good name is for the root of the Dashboard App (and other apps as they are added). dashboardAppmaybe? For now I just renamed todashboard`. It's just that without any context it feels confusing. Oh well, no better ideas for right now.

kimjoar · 2017-10-13T18:00:00Z

src/core_plugins/kibana/public/dashboard/actions/embeddables.js

+export function renderEmbeddable(embeddableHandler, panelElement, panelId, containerApi) {
+  return (dispatch, getState) => {
+    const { dashboardState } = getState();
+    const panelState = dashboardState.panels[panelId];


This isn't using selectors, so it's "locking down" the state shape. Is there a reason for not injecting it into the action instead? E.g.

diff --git a/src/core_plugins/kibana/public/dashboard/actions/embeddables.js b/src/core_plugins/kibana/public/dashboard/actions/embeddables.js index 236cd9a79..df4d1e70f 100644 --- a/src/core_plugins/kibana/public/dashboard/actions/embeddables.js +++ b/src/core_plugins/kibana/public/dashboard/actions/embeddables.js @@ -22,22 +22,19 @@ export const embeddableRenderError = * @param containerApi {ContainerAPI} * @return {function(*, *)} */ -export function renderEmbeddable(embeddableHandler, panelElement, panelId, containerApi) { +export function renderEmbeddable(embeddableHandler, panelState, panelElement, containerApi) { return (dispatch, getState) => { - const { dashboardState } = getState(); - const panelState = dashboardState.panels[panelId]; - if (!embeddableHandler) { - dispatch(embeddableRenderError(panelId, new Error(`Invalid embeddable type "${panelState.type}"`))); + dispatch(embeddableRenderError(panelState.panelIndex, new Error(`Invalid embeddable type "${panelState.type}"`))); return; } return embeddableHandler.render(panelElement, panelState, containerApi) .then(embeddable => { - return dispatch(embeddableRenderFinished(panelId, embeddable)); + return dispatch(embeddableRenderFinished(panelState.panelIndex, embeddable)); }) .catch(error => { - dispatch(embeddableRenderError(panelId, error)); + dispatch(embeddableRenderError(panelState.panelIndex, error)); }); }; } diff --git a/src/core_plugins/kibana/public/dashboard/panel/dashboard_panel.js b/src/core_plugins/kibana/public/dashboard/panel/dashboard_panel.js index 5181086a9..2a614d121 100644 --- a/src/core_plugins/kibana/public/dashboard/panel/dashboard_panel.js +++ b/src/core_plugins/kibana/public/dashboard/panel/dashboard_panel.js @@ -7,7 +7,7 @@ import { PanelError } from './panel_error'; export class DashboardPanel extends React.Component { async componentDidMount() { - this.props.renderEmbeddable(this.panelElement); + this.props.renderEmbeddable(this.panelElement, this.props.panel); } toggleExpandedPanel = () => { diff --git a/src/core_plugins/kibana/public/dashboard/panel/dashboard_panel_container.js b/src/core_plugins/kibana/public/dashboard/panel/dashboard_panel_container.js index 7cf495a9b..130c95558 100644 --- a/src/core_plugins/kibana/public/dashboard/panel/dashboard_panel_container.js +++ b/src/core_plugins/kibana/public/dashboard/panel/dashboard_panel_container.js @@ -19,6 +19,7 @@ import { getEmbeddableEditUrl, getMaximizedPanelId, getEmbeddableError, + getPanel } from '../reducers'; const mapStateToProps = ({ dashboardState }, { panelId }) => { @@ -30,11 +31,12 @@ const mapStateToProps = ({ dashboardState }, { panelId }) => { viewOnlyMode: getFullScreenMode(dashboardState) || getViewMode(dashboardState) === DashboardViewMode.VIEW, isExpanded: getMaximizedPanelId(dashboardState) === panelId, + panel: getPanel(dashboardState, panelId) }; }; const mapDispatchToProps = (dispatch, { embeddableHandler, panelId, getContainerApi }) => ({ - renderEmbeddable: (panelElement) => dispatch(renderEmbeddable(embeddableHandler, panelElement, panelId, getContainerApi())), + renderEmbeddable: (panelElement, panel) => dispatch(renderEmbeddable(embeddableHandler, panel, panelElement, getContainerApi())), onDeletePanel: () => { dispatch(deletePanel(panelId)); dispatch(destroyEmbeddable(panelId, embeddableHandler));

(PS! I didn't run this, so I don't actually know if it works — time was running away from me)

I can switch to using selectors, though it sounded like there wasn't agreement on whether or not we should consider getState an anti-pattern. I'm not really a fan of passing state through just to get it back in mapDispatchToProps, when the component doesn't actually need it.

Ugh, turns out that creates a circular dependency (at least I suspect, though the failure error is slightly vague, it makes sense since now actions requires reducers which requires actions).

/shrug I guess I'll just implement your suggestion for the time being. Though I think it might be a mistake to put selectors in the reducer files because of this.

kimjoar · 2017-10-13T18:03:31Z

src/core_plugins/kibana/public/dashboard/grid/dashboard_grid.js

    } = this.props;

    // Part of our unofficial API - need to render in a consistent order for plugins.
-    const panelsInOrder = panels.slice(0);
+    const panelsInOrder = Object.keys(panels).map(key => panels[key]);


Hm, weird. We're including babel-polyfill, so I expected it to be there. Might be something with our setup. Works for me.

kimjoar · 2017-10-13T18:08:03Z

src/core_plugins/kibana/public/dashboard/grid/dashboard_grid.js

@@ -145,12 +148,10 @@ export class DashboardGrid extends React.Component {
 }

 DashboardGrid.propTypes = {
-  isFullScreenMode: PropTypes.bool.isRequired,
-  panels: PropTypes.array.isRequired,
+  panels: PropTypes.object.isRequired,


Ah, my bad. I think I misread the code at first. Yeah, that's probably not easy with shape, but looks like there's a objectOf now that can be used. Maybe something like PropTypes.objectOf(PropTypes.shape(...)). But I'm fine with not doing it.

discover was not returning a promise

stacey-gammon · 2017-10-13T20:38:30Z

Thanks for the follow up! I just posted some more tests and a fix for the issue @kobelb found (great catch!) but ended up finding another issue. The tests found it so should break the build at this point, but I'll tackle it in a separate commit because it will require a bit more thought.

The crux of the issue is that maximizing and then minimizing a panel renders the same embeddable, with the same panelId, twice. Minimizing it causes it to unmount (since it's a separate component), which destroys the embeddable. The grid, which has a panel that is referencing the same embeddable information and panelId, is now destroyed, and since we intentionally don't re-render the grid so max/minning doesn't cost a bunch of render time, it loses the embeddable object from state.

The ideal fix I think is to max and min a panel via css so nothing is re-rendered - not even the expanded panel, like it is now. This is better anyway because, take the situation where you have a panel that was rendered at time x. You expand it, it's re-rendered at time Y and so is slightly different. You minimize it and now it appears back at time x in the grid.

Another fix, if that ends up being too complicated, is that we only remove embeddable objects if there is no longer a panel that is referencing it, but this seems fragile as well. The coupling of embeddables and panels could be improved, perhaps by moving some of the state around to better reflect their relationship.

…renders

…d refresh

…/dashboard-redux

stacey-gammon · 2017-10-15T17:30:11Z

Failed on:

:04:28          │ debg  --- tryForTime failed again with the same message  ...
23:04:28          │ debg  Taking screenshot "/var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-selenium/test/functional/screenshots/failure/dashboard app dashboard state Directly modifying url updates dashboard state for panel size parameters.png"
23:04:28        └- ✖ fail: "dashboard app dashboard state Directly modifying url updates dashboard state for panel size parameters"
23:04:28        │        tryForTime timeout: Error: expected 1005 to be above 1015
23:04:28        │           at Assertion.assert (/var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-selenium/node_modules/expect.js/index.js:96:13)
23:04:28        │           at Assertion.greaterThan.Assertion.above (/var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-selenium/node_modules/expect.js/index.js:297:10)
23:04:28        │           at Function.greaterThan (/var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-selenium/node_modules/expect.js/index.js:499:17)
23:04:28        │           at /var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-selenium/test/functional/apps/dashboard/_dashboard_state.js:56:53
23:04:28        │           at undefined.next (native)
23:04:28        │           at step (/var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-selenium/test/functional/apps/dashboard/_dashboard_state.js:4:1)
23:04:28        │           at /var/lib/jenkins/workspace/elastic+kibana+

While screenshot:

This passed locally. Not sure where the extra padding on the right came from. Locally, it expands to 100%.

Jenkins, test this.

stacey-gammon · 2017-10-20T11:00:39Z

Looks like a flaky failing test. Will need to investigate. Screenshot wasn't uploaded unfortunately.

10:25:16            └- ✓ pass 
10:25:16        └-: saved search object edit menu
10:25:16          └-> "before all" hook
10:25:16            │ debg  click Discover tab
10:25:16            │ debg  clickSelector(a[href*='discover'])
10:25:16            │ debg  TestSubjects.exists(top-nav)
10:25:16            │ debg  existsByDisplayedByCssSelector [data-test-subj~="top-nav"]
10:25:16            │ debg  isGlobalLoadingIndicatorHidden
10:25:16            │ debg  findByCssSelector [data-test-subj="globalLoadingIndicator"].ng-hide
10:25:18            │ debg  TestSubjects.find(field-bytes)
10:25:18            │ debg  in displayedByCssSelector: [data-test-subj~="field-bytes"]
10:25:18            │ debg  TestSubjects.find(fieldToggle-bytes)
10:25:18            │ debg  in displayedByCssSelector: [data-test-subj~="fieldToggle-bytes"]
10:25:18            │ debg  TestSubjects.find(discoverSaveButton)
10:25:18            │ debg  in displayedByCssSelector: [data-test-subj~="discoverSaveButton"]
10:25:18            │ debg  --saveSearch button clicked
10:25:19            │ debg  --find save button
10:25:19            │ debg  TestSubjects.find(discoverSaveSearchButton)
10:25:19            │ debg  in displayedByCssSelector: [data-test-subj~="discoverSaveSearchButton"]
10:25:19            │ debg  isGlobalLoadingIndicatorVisible
10:25:19            │ debg  TestSubjects.exists(globalLoadingIndicator)
10:25:19            │ debg  existsByDisplayedByCssSelector [data-test-subj~="globalLoadingIndicator"]
10:25:19            │ debg  isGlobalLoadingIndicatorHidden
10:25:19            │ debg  findByCssSelector [data-test-subj="globalLoadingIndicator"].ng-hide
10:25:20            │ debg  clickToastOK
10:25:20            │ debg  click Dashboard tab
10:25:20            │ debg  clickSelector(a[href*='dashboard'])
10:25:20            │ debg  TestSubjects.exists(top-nav)
10:25:20            │ debg  existsByDisplayedByCssSelector [data-test-subj~="top-nav"]
10:25:20            │ debg  isGlobalLoadingIndicatorHidden
10:25:20            │ debg  findByCssSelector [data-test-subj="globalLoadingIndicator"].ng-hide
10:25:20            │ debg  TestSubjects.find(dashboardAddPanelButton)
10:25:20            │ debg  in displayedByCssSelector: [data-test-subj~="dashboardAddPanelButton"]
10:25:21            │ debg  TestSubjects.find(addSavedSearchTab)
10:25:21            │ debg  in displayedByCssSelector: [data-test-subj~="addSavedSearchTab"]
10:25:21            │ debg  TestSubjects.find(savedObjectFinderSearchInput)
10:25:21            │ debg  in displayedByCssSelector: [data-test-subj~="savedObjectFinderSearchInput"]
10:25:21            │ debg  clickByPartialLinkText(my search)
10:25:21            │ debg  find.byPartialLinkText(my search)
10:25:21            │ debg  Taking screenshot "/var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-selenium/test/functional/screenshots/failure/dashboard app dashboard panel controls panel edit controls saved search object edit menu "before all" hook.png"
10:25:21          └- ✖ fail: "dashboard app dashboard panel controls panel edit controls saved search object edit menu "before all" hook"
10:25:21          │        [POST http://localhost:9515/session/225b8118f65fe077b6368b378a01f024/element/0.5259794685949579-28/click] stale element reference: element is not attached to the page document
10:25:21          │         (Session info: chrome=59.0.3071.115)
10:25:21          │         (Driver info: chromedriver=2.32.498513 (2c63aa53b2c658de596ed550eb5267ec5967b351),platform=Linux 3.10.0-514.26.2.el7.x86_64 x86_64)
10:25:21          │         at Server._post (test/functional/services/remote/verbose_remote_logging.js:15:21)
10:25:21          │         at runRequest (node_modules/leadfoot/Session.js:92:40)
10:25:21          │         at getFinalValue (node_modules/dojo/Promise.js:258:35)
10:25:21          │         at node_modules/dojo/Promise.js:272:24
10:25:21          │         at node_modules/dojo/Promise.js:156:41
10:25:21          │         at runCallbacks (node_modules/dojo/Promise.js:19:22)
10:25:21          │         at node_modules/dojo/Promise.js:103:21
10:25:21          │         at run (node_modules/dojo/Promise.js:51:33)
10:25:21          │         at node_modules/dojo/nextTick.js:35:17
10:25:21          │         at _combinedTickCallback (internal/process/next_tick.js:73:7)
10:25:21          │         at process._tickDomainCallback (internal/process/next_tick.js:128:9)
10:25:21          │       
10:25:21          │       
10:25:21            │

…d loading results

…still use selectors

w33ble

Some minor misuse of createAction

w33ble · 2017-10-23T16:55:23Z

src/core_plugins/kibana/public/dashboard/actions/panels.js

@@ -0,0 +1,17 @@
+import { createAction } from 'redux-actions';
+
+export const deletePanel = createAction('DELETE_PANEL', panelId => panelId);


This is not required, the default function in createAction is _.indentity, so you can simply write this (and other actions like this that simply pass there parameters to the reducer) like so:

export const deletePanel = createAction('DELETE_PANEL')

The only time you need to provide the function is if you need to map multiple arguments into a single object, or otherwise do something with the action's arguments before passing the values to the reducer (as you do below).

w33ble · 2017-10-23T16:57:26Z

src/core_plugins/kibana/public/dashboard/actions/view.js

@@ -0,0 +1,6 @@
+import { createAction } from 'redux-actions';
+
+export const updateViewMode = createAction('UPDATE_VIEW_MODE', viewMode => viewMode);


See my comment in the panels actions; all of these action creators don't need to the second argument.

stacey-gammon · 2017-10-23T17:57:11Z

Thanks @w33ble, I incorporated your changes into #14518.

Unfortunately I noticed some really weird merge issues with this PR and I stopped getting all the updates from master. I couldn't think of a way to fix it other than copying over all the commits into a fresh PR. Closing this one in favor of that.

stacey-gammon added Feature:Dashboard Dashboard related features :Sharing chore labels Sep 27, 2017

stacey-gammon force-pushed the chore/dashboard-redux branch 4 times, most recently from d876c70 to 54216b6 Compare September 28, 2017 01:40

stacey-gammon commented Sep 28, 2017

View reviewed changes

stacey-gammon force-pushed the chore/dashboard-redux branch 3 times, most recently from adbd492 to 781daef Compare September 28, 2017 15:03

stacey-gammon mentioned this pull request Sep 28, 2017

[WIP] Add redux to dashboard state management #14103

Closed

stacey-gammon added the v6.1.0 label Sep 28, 2017

stacey-gammon force-pushed the chore/dashboard-redux branch 4 times, most recently from 051c2f5 to aa60e38 Compare October 2, 2017 15:54

bmcconaghy reviewed Oct 2, 2017

View reviewed changes

w33ble suggested changes Oct 2, 2017

View reviewed changes

w33ble reviewed Oct 2, 2017

View reviewed changes

stacey-gammon force-pushed the chore/dashboard-redux branch 2 times, most recently from fb9f96d to 9d963c8 Compare October 3, 2017 20:47

kimjoar approved these changes Oct 13, 2017

View reviewed changes

Fix issue plus tests

d77c0ee

discover was not returning a promise

Make expanding a panel purely a css modification which avoids all re-…

e7bf212

…renders

stacey-gammon force-pushed the chore/dashboard-redux branch from 86367a8 to e7bf212 Compare October 13, 2017 22:56

Found another bug with initial state not being set correctly on a har…

3c20f1d

…d refresh

stacey-gammon force-pushed the chore/dashboard-redux branch from 7e3e647 to 3c20f1d Compare October 14, 2017 01:58

stacey-gammon added 2 commits October 14, 2017 18:41

Merge branch 'master' of https://github.com/elastic/kibana into chore…

608a099

…/dashboard-redux

Remove check for change handlers now that the event handler bug is fixed

802c34c

stacey-gammon force-pushed the chore/dashboard-redux branch from 740feab to 57d5f40 Compare October 15, 2017 18:08

stacey-gammon added 4 commits October 18, 2017 15:30

rename dashboardState => dashboard for reducers and redux state tree

3f84212

Remove unnecessary top level describe in jest tests

c6c5a9d

Navigate back to landing page at the end of the newly added test suite

d20bba2

Fix lint errors

4e884a4

stacey-gammon force-pushed the chore/dashboard-redux branch 2 times, most recently from 2d4f657 to 4e884a4 Compare October 20, 2017 10:12

Stabilize flaky tests by waiting until saved object search is finishe…

e4de737

…d loading results

stacey-gammon mentioned this pull request Oct 20, 2017

Can't use Object.values in our test suite #14497

Closed

stacey-gammon added 5 commits October 20, 2017 14:56

use spread operator over object.assign

76034f8

Don't leak subscriptions to the store.

3b3ccf9

use selectors to grab dashboard panel off state.

3175061

Remove use of getState in dispatcher to avoid circular reference and …

fe86a5c

…still use selectors

Merge with master

025a62e

w33ble suggested changes Oct 23, 2017

View reviewed changes

stacey-gammon mentioned this pull request Oct 23, 2017

Introduce redux into dashboard #14518

Merged

stacey-gammon closed this Oct 23, 2017

stacey-gammon deleted the chore/dashboard-redux branch November 6, 2017 19:28

		@@ -0,0 +1,12 @@
		import { createStore, applyMiddleware } from 'redux';

		@@ -0,0 +1,17 @@
		import { createAction } from 'redux-actions';

		export const deletePanel = createAction('DELETE_PANEL', panelId => panelId);

		@@ -0,0 +1,6 @@
		import { createAction } from 'redux-actions';

		export const updateViewMode = createAction('UPDATE_VIEW_MODE', viewMode => viewMode);

		});

		uiRoutes

Introduce redux state management for react portion of dashboard #14199

Introduce redux state management for react portion of dashboard #14199

Conversation

stacey-gammon commented Sep 27, 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

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

stacey-gammon commented Oct 2, 2017

stacey-gammon commented Oct 2, 2017

Choose a reason for hiding this comment

w33ble Oct 2, 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

w33ble commented Oct 2, 2017

stacey-gammon commented Oct 2, 2017

kimjoar 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimjoar Oct 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimjoar Oct 13, 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

Choose a reason for hiding this comment

stacey-gammon commented Oct 13, 2017 • edited Loading

stacey-gammon commented Oct 15, 2017

stacey-gammon commented Oct 20, 2017

w33ble left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stacey-gammon commented Oct 23, 2017

stacey-gammon commented Sep 27, 2017 •

edited

Loading

w33ble Oct 2, 2017 •

edited

Loading

kimjoar Oct 13, 2017 •

edited

Loading

kimjoar Oct 13, 2017 •

edited

Loading

stacey-gammon commented Oct 13, 2017 •

edited

Loading