Remove the need to pass state and parentUiState to dashboard_panel directive #9769

stacey-gammon · 2017-01-06T17:37:24Z

This improves isolation of the panel and makes state changes clearer.

Along the way I simplified the logic for loading the saved object and once simplified, I didn't think it required a separate file so brought the logic inside the directive.

Also made note of where #9523 is creeping in. I was originally going to fix it in this PR but since how to fix it isn't clear yet, this PR remains a non-functional change.

weltenwort

I really like the way you create the child ui state a lot better. 👍 🎉 The saveState property also looks quite elegant.

But I have to say that I'm not in favor of inlining the loader that way. Since the closure environment is a lot bigger that way it is hard for me to keep it all in working memory when reading through dashboard_panel.js. I thought the previous loaders were a lot easier to understand (and probably to test as well). loadPanelProvider seemed to have been a misnomer though, with loadSavedObjectProvider being more accurate.

Moving the two $watchCollection calls out of the searchLoaderProvider on the other hand makes sense.

Maybe there is a middle-ground to be had where we can move maintain your sensible sequence of "loadSavedObject", "initializeSavedObject" and "initializePanel" while not relying on a huge closure. I feel I need to dwell on this a bit and will post a follow-up later today. 🤔

weltenwort · 2017-01-13T17:10:11Z

src/core_plugins/kibana/public/dashboard/directives/dashboard_panel.js

+         */
+        let postLoadInit;
+
+        if ($scope.panel.type === 'search') {


The problem I have with this explicit conditional is that it does not scale well to new panel types:

Previously, when adding a new panel type, a new loader would just have to be created and added to the declarative panelTypes mapping in loadPanelProvider. The panel code itself would not have to be changed.

This conditional though would grow with each new panel type and probably become a lot harder to read.

Got rid of the conditional. New types just need to register a loader in getObjectLoadersForDashboard. No good place right now for custom post initialization code, but ideally there wouldn't need to be any.

stacey-gammon · 2017-01-13T20:34:20Z

What do you think about this new version @weltenwort? I extracted the load_saved_object code and removed the need for scope by pushing the "postInit" function stuff into initializePanel which is probably a better spot for it anyhow, since that is "postInit" code.

I created a new service for the loaders, so to add a new type all you have to do is add one to that service. Any custom initialization of the saved object must, for now, go in the directive.

I'd like to improve that latter requirement, but I think this PR is sufficient and that can be a Step 3. Plus I don't have a clear idea right now how to improve the initialization code.

kobelb

Awesome job separating the dashboard_panel directive out from requiring the parent uiState, I really like this improvement.

kobelb · 2017-01-13T21:37:45Z

src/core_plugins/kibana/public/dashboard/components/panel/lib/load_saved_object.js

+export function loadSavedObject(loaders, panel) {
+  const loader = loaders.find((loader) => loader.type === panel.type);
+  if (!loader) {
+    throw `No loader for object of type ${panel.type}`;


Even though Javascript lets us throw whatever we want, it's good practice to throw a new Error('error message') as opposed to a string.

kobelb · 2017-01-13T21:43:43Z

src/core_plugins/kibana/public/dashboard/directives/dashboard_panel.js

@@ -1,15 +1,19 @@
 import _ from 'lodash';
 import 'ui/visualize';
 import 'ui/doc_table';
-import { loadPanelProvider } from 'plugins/kibana/dashboard/components/panel/lib/load_panel';
+import 'plugins/kibana/dashboard/services/get_object_loaders_for_dashboard.factory';


I dunno how I feel about the .factory part of the filename. Was it your intention for us to do this for all of the different angular 'types'?

This was motivated by some conversations with @w33ble and a new style pattern the Management team has been experimenting with. This morning I decided I'm on the fence with whether or not I should be incorporating this new pattern, read more here: #9859 (comment)

Once I hear back from @w33ble and @cjcenizal on that comment I'll decide whether to revert the .factory naming scheme.

FWIW I do like the pattern a lot but since it's not fully adopted by the team, it does seem a bit premature, especially with the last email that went around about consistency trumping new patterns until a certain # of criteria is hit.

Here too, reverting this change to avoid introducing new patterns that aren't yet embraced by the team as a whole.

kobelb · 2017-01-13T21:48:12Z

src/core_plugins/kibana/public/dashboard/directives/dashboard_panel.js

-.directive('dashboardPanel', function (savedVisualizations, savedSearches, Notifier, Private, $injector) {
-  const loadPanel = Private(loadPanelProvider);
+.directive('dashboardPanel', function ($injector) {
+  const Private = $injector.get('Private');


What made you switch to using the $injector service directly?

another new style suggestion from the Management team. Once you are only using the injector, you don't need to wrap the class in Private, so it'll make getting rid of Private easier.

Well, FilterManagerProvider is still wrapped below. Isn't the only reason you get around using Private on getObjectLoadersForDashboard the fact that it has been globally registered under that name? That is still tightly coupled to Angular's DI, then.

Yea, I think the idea is to stop using nameless functions that require Private, and explicitly register them in angular. The more I think about this specific change though, the more I am unsure.

I was originally under the assumption that if I got rid of the parameters and only passed $Injector, I could avoid wrapping dashboardPanel with Private but that doesn't make sense because dashboardPanel is a directive, first off, and second off, is registered with angular, and no class registered with angular would require Private.

After going back to the original discussion with @w33ble I think the idea is to use $injector only for nameless functions, not for things already registered with angular. Does that sound right, @w33ble? So this should go back to it's previous version?

That is still tightly coupled to Angular's DI

Everything will be, you can't use Angular without the DI 😢 .

There's actually 2 things going on here though. First is the removal of Private, which I think is a universal desire.

The second is the use of $injector to make it clear this is Angular stuff, to allow the code to be minified without duplicating all the dependencies, and I guess tangentially to make the code still look like the Private code. This change is very much up for debate, and was just an idea that was thrown out when discussing the Angular styleguide (which is also kind of in flux). Sounds like folks aren't too keen on this idea, so maybe it's best to remove it for now and discuss it outside this PR.

I think the idea is to use $injector only for nameless functions, not for things already registered with angular.

Actually, $injector only works for named, registered Angular code. It doesn't work the same way the Private does.

Okay folks, I reverted to the old variation. Discussed with @cjcenizal and @w33ble and decided to move the debate around the new $injector style to a separate issue and pull out of here, in order to stay consistent with the current code, until the new pattern is embraced as a whole by the team.

kobelb · 2017-01-13T22:10:50Z

src/core_plugins/kibana/public/dashboard/directives/dashboard_panel.js

+        // This causes changes to a saved search to be hidden, but also allows
+        // the user to locally modify and save changes to a saved search only in a dashboard.
+        // See https://github.com/elastic/kibana/issues/9523 for more details.
+        $scope.panel.columns = $scope.panel.columns || $scope.savedObj.columns;


This used to only happen for saved searches. Are $scope.panel.columns and $scope.panel.sort only being set so that we can watch them and know when to call saveState?

True, maybe a if ($scope.savedObj.type === 'search') { ... } would be appropriate? And then line 97 could also read if ($scope.savedObj.type === 'visualization') for symmetry.

Yea, I figured it was just a noop for visualizations, but I can wrap in an if as well.

weltenwort

Since there was a if ($scope.savedObj.vis) in initializePanel anyway, moving the handler registration there makes sense. Now it's all in one place, at least.

And nice job reusing loader.type! 👍

weltenwort · 2017-01-16T12:00:45Z

src/core_plugins/kibana/public/dashboard/directives/dashboard_panel.js

+        // This causes changes to a saved search to be hidden, but also allows
+        // the user to locally modify and save changes to a saved search only in a dashboard.
+        // See https://github.com/elastic/kibana/issues/9523 for more details.
+        $scope.panel.columns = $scope.panel.columns || $scope.savedObj.columns;


True, maybe a if ($scope.savedObj.type === 'search') { ... } would be appropriate? And then line 97 could also read if ($scope.savedObj.type === 'visualization') for symmetry.

stacey-gammon · 2017-01-18T19:25:53Z

@weltenwort and @kobelb should be all good to go for a final look, thanks!

get rid of parentUiState

Removing the new patterns from this PR to preserve consistency.

kobelb

LGTM

weltenwort

LGTM too

…rective Backports PR #9769 **Commit 1:** - Remove need for passing state to dashboard_panel directive. get rid of parentUiState * Original sha: e97cb33 * Authored by Stacey Gammon <[email protected]> on 2017-01-05T21:05:40Z **Commit 2:** Address code review comments * Original sha: 4c7f0be * Authored by Stacey Gammon <[email protected]> on 2017-01-13T20:29:45Z **Commit 3:** Only do savedSearch specific init stuff for searches * Original sha: 76149ed * Authored by Stacey Gammon <[email protected]> on 2017-01-16T15:21:42Z **Commit 4:** Throw Error object, not string * Original sha: 31713b0 * Authored by Stacey Gammon <[email protected]> on 2017-01-17T15:25:47Z **Commit 5:** Remove new patterns from this PR Removing the new patterns from this PR to preserve consistency. * Original sha: 65131d0 * Authored by Stacey Gammon <[email protected]> on 2017-01-17T20:15:07Z

…rective (elastic#9769) * - Remove need for passing state to dashboard_panel directive. get rid of parentUiState * Address code review comments * Only do savedSearch specific init stuff for searches * Throw Error object, not string * Remove new patterns from this PR Removing the new patterns from this PR to preserve consistency.

…rective (#9769) (#9980) * - Remove need for passing state to dashboard_panel directive. get rid of parentUiState * Address code review comments * Only do savedSearch specific init stuff for searches * Throw Error object, not string * Remove new patterns from this PR Removing the new patterns from this PR to preserve consistency.

stacey-gammon requested review from weltenwort and cjcenizal January 6, 2017 18:58

stacey-gammon added Feature:Dashboard Dashboard related features review labels Jan 6, 2017

stacey-gammon requested review from kobelb and removed request for cjcenizal January 6, 2017 19:29

tbragin added the :Sharing label Jan 8, 2017

stacey-gammon force-pushed the saved-search-refresh-bug branch from 2390122 to 2fdc280 Compare January 9, 2017 14:43

stacey-gammon assigned kobelb and weltenwort Jan 9, 2017

stacey-gammon force-pushed the saved-search-refresh-bug branch from 2fdc280 to 7b9eb00 Compare January 10, 2017 15:28

weltenwort reviewed Jan 13, 2017

View reviewed changes

kobelb reviewed Jan 13, 2017

View reviewed changes

weltenwort reviewed Jan 16, 2017

View reviewed changes

stacey-gammon force-pushed the saved-search-refresh-bug branch 2 times, most recently from b726936 to 67e102f Compare January 17, 2017 15:26

stacey-gammon added the v5.3.0 label Jan 18, 2017

stacey-gammon added 5 commits January 19, 2017 10:18

- Remove need for passing state to dashboard_panel directive.

e97cb33

get rid of parentUiState

Address code review comments

4c7f0be

Only do savedSearch specific init stuff for searches

76149ed

Throw Error object, not string

31713b0

Remove new patterns from this PR

65131d0

Removing the new patterns from this PR to preserve consistency.

stacey-gammon force-pushed the saved-search-refresh-bug branch from 3b7fec3 to 65131d0 Compare January 19, 2017 15:18

kobelb approved these changes Jan 19, 2017

View reviewed changes

weltenwort approved these changes Jan 19, 2017

View reviewed changes

stacey-gammon merged commit 2fccac6 into elastic:master Jan 19, 2017

elastic-jasper mentioned this pull request Jan 19, 2017

[5.x] Remove the need to pass state and parentUiState to dashboard_panel directive #9965

Closed

stacey-gammon mentioned this pull request Jan 20, 2017

[5.x] Remove the need to pass state and parentUiState to dashboard_panel - Manual Backport #9980

Merged

stacey-gammon deleted the saved-search-refresh-bug branch January 20, 2017 18:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove the need to pass state and parentUiState to dashboard_panel directive #9769

Remove the need to pass state and parentUiState to dashboard_panel directive #9769

stacey-gammon commented Jan 6, 2017

weltenwort left a comment

weltenwort Jan 13, 2017

stacey-gammon Jan 13, 2017

stacey-gammon commented Jan 13, 2017

kobelb left a comment

kobelb Jan 13, 2017

kobelb Jan 13, 2017

stacey-gammon Jan 16, 2017

stacey-gammon Jan 17, 2017

kobelb Jan 13, 2017

stacey-gammon Jan 16, 2017

weltenwort Jan 16, 2017

stacey-gammon Jan 16, 2017

w33ble Jan 17, 2017 •

edited

Loading

stacey-gammon Jan 17, 2017

kobelb Jan 13, 2017

weltenwort Jan 16, 2017

stacey-gammon Jan 16, 2017

weltenwort left a comment

weltenwort Jan 16, 2017

stacey-gammon commented Jan 18, 2017

kobelb left a comment

weltenwort left a comment

Remove the need to pass state and parentUiState to dashboard_panel directive #9769

Remove the need to pass state and parentUiState to dashboard_panel directive #9769

Conversation

stacey-gammon commented Jan 6, 2017

weltenwort 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 Jan 13, 2017

kobelb 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

Choose a reason for hiding this comment

w33ble Jan 17, 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

weltenwort left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stacey-gammon commented Jan 18, 2017

kobelb left a comment

Choose a reason for hiding this comment

weltenwort left a comment

Choose a reason for hiding this comment

w33ble Jan 17, 2017 •

edited

Loading