[indexPatterns/create] refactor time field options #11996

spalger · 2017-05-25T01:00:32Z

In #11409 we added two new possible options to the "Time Filter field name" drop down. These options display a message that "there are no time fields" or "I don't want to use the time filter". These new options were added to the controller.timeFields array, which is where the other options were, but they don't actually represent time fields.

Summary of updates:

controller.timeFields is now a list of options, specifically created to populate the select box in the view. They are now stored at controller.timeFieldOptions and each option has a display property (which will be rendered in the view) and optionally a fieldName property. The selected option, controller.formValues.timeFieldOption is checked for a fieldName property when the Index Pattern is created.
controller.fetchFieldsError is now controller.timeFieldOptionsError
controller.refreshFieldList() is now controller.refreshTimeFieldOptions()

In elastic#11409 we added two new possible options to the "Time Filter field name" drop down. These options display a message that "there are no time fields" or "I don't want to use the time filter". These new options were added to the `controller.timeFields` array, which is where the other options were, but they don't actually represent time fields. This change updates `controller.timeFields` to instead be a list of options, specifically created to populate the select box in the view. They are now stored at `controller.timeFieldOptions` and each option has a `display` property (which will be rendered in the view) and optionally a `fieldName` property. The selected option, `controller.formValues.timeFieldOption` is checked for a `fieldName` property when the Index Pattern is created.

…s/time-field-options

cjcenizal

Looks like a bit of a bug here... not sure what to suggest as a solution, though. Looks like this branch also needs a rebase.

cjcenizal · 2017-05-25T03:03:03Z

...ugins/kibana/public/management/sections/indices/create_index_pattern/create_index_pattern.js

-    let fetchFieldsError;
-    let dateFields;
+  const getTimeFieldOptions = () => {
+    this.timeFieldOptions = null;


I get this error when I load this page:

It looks like it's because this line sets timeFieldOptions to null but then it never gets set to an array.

Doing this prevents the time field from rendering as invalid when there is no matching index pattern (which has it's own error display) and when the fields are loading

spalger · 2017-05-25T18:15:20Z

@cjcenizal @ycombinator I fixed the bugs, cleaned up the rest of the renaming, and isolated the side effects in refreshTimeFieldOptions() so that my next pr will be cleaner. Can you take another look?

cjcenizal

Awesome! Functionality looks like it's still the same as what currently exists, and the code is much clearer. I especially love the new comments. Just had some questions and small polish-level suggestions.

cjcenizal · 2017-05-25T18:32:56Z

...ugins/kibana/public/management/sections/indices/create_index_pattern/create_index_pattern.js

+          return;
+        }
+
+        const restoredOption = findTimeFieldOption(prevOption);


Can we add a brief comment to describe these 3 lines?

// Persist the selected state in the UI.

cjcenizal · 2017-05-25T18:33:28Z

...ugins/kibana/public/management/sections/indices/create_index_pattern/create_index_pattern.js

-    }
+    return this.timeFieldOptions.find(option => (
+      // comparison is not done with _.isEqual() because options get a unique
+      // `$$hashKey` tag attached to them by ng-repeat


❤️ this comment

cjcenizal · 2017-05-25T18:38:23Z

...ugins/kibana/public/management/sections/indices/create_index_pattern/create_index_pattern.js

+  const pickDefaultTimeFieldOption = () => {
+    const noOptions = this.timeFieldOptions.length === 0;
+    const fieldOptions = this.timeFieldOptions.filter(option => !!option.fieldName);
+    const infoOptions = this.timeFieldOptions.filter(option => !option.fieldName);


What are infoOptions? Maybe it's better to describe these as dummyOptions or nonSelectableOptions to make their role clearer in this context?

And instead of deriving this state based on fieldName, can we just add a isSelectable: true property to the options on line 82? This would make the logic clearer:

const selectableOptions = this.timeFieldOptions.filter(option => option.isSelectable); const nonSelectableOptions = this.timeFieldOptions.filter(option => !option.isSelectable);

Well, they're all selectable, they are really just "non-field" options. I could use that as the description rather than "info" if you think that's better.

cjcenizal · 2017-05-25T18:39:01Z

...ugins/kibana/public/management/sections/indices/create_index_pattern/create_index_pattern.js

+    const noOptions = this.timeFieldOptions.length === 0;
+    const fieldOptions = this.timeFieldOptions.filter(option => !!option.fieldName);
+    const infoOptions = this.timeFieldOptions.filter(option => !option.fieldName);
+    const tooManyOptions = fieldOptions.length > 1 || infoOptions.length > 1;


What does it mean for there to be too many options? Too many to do what? Wondering if there's a name that can communicate the intent more clearly.

Too many options to choose the default. If there are multiple field or nonField options then we can't pick the default, the user has to choose something... open to suggestions for better names.

spalger · 2017-05-25T18:56:12Z

@cjcenizal I tried to clear up the intention behind the variables in pickDefaultTimeFieldOption(), can you take another look?

chrisronline

Looks great overall! Just some minor questions/suggestions!

chrisronline · 2017-05-25T18:49:42Z

...ugins/kibana/public/management/sections/indices/create_index_pattern/create_index_pattern.js

+    this.timeFieldOptions = [];
+    this.timeFieldOptionsError = null;
+    this.formValues.timeFieldOption = null;
+    getTimeFieldOptions()


Is there a reason we aren't opting for the async/await pattern here and using promises directly?

Unfortunately, async/await don't work with angular for the same reason we can't use the global Promise contrustor :/ See #11632 (comment)

Interesting. Why doesn't a simple $scope.$apply() not work in those scenarios? Or is that not recommended for other reasons?

Manually triggering digest cycles is really a last resort option and can lead to all sorts of issues even in simple apps.

That's interesting. I'd love to hear more about this. Do you have any resources and/or past github issues around this?

I'm very interested in understanding the concerns around explicit $scope.$apply(). I've always considered it a valid option and the best option when dealing with async operations outside of angular.

I agree that sometimes $scope.$apply() is the only way to inter-op between angular and non-angular async contexts. My concern is that using $scope.$apply() when not absolutely necessary leads to trouble. We did this in Kibana 3 and ended up with more than a couple PR's that just converted a $scope.$apply() to if (!$scope.$$phase) $scope.$apply() because things started to get out of hand. I'm just saying that if we can avoid using $scope.$apply() (like by using native angular promises) then I think we should.

I agree, we should avoid using $apply where possible. I think the way to do that is to follow a few simple rules:

Prefer Angular DOM event handlers like ng-click over event handlers (like $(element).on('click', () => {}) and element.addEventListener('click', () => {}).'

Prefer Angular wrappers around global objects like $document and $window.

Prefer Angular wrappers around async APIs like $q, $timeout, and $interval.

There are probably a few others we can dig up by Googling but the general theme is to just use whatever tools Angular makes available which will tie into the $digest cycle under the hood. These are situations where $apply is inappropriate.

This leaves us with the situations where $apply is appropriate: anywhere these tools can't be used. I think if we can confidently assess a call stack and state that it will never touch any of these tools, and therefore never trigger a $digest cycle on its own, then we can safely use $apply. Thoughts @spalger and @chrisronline ?

@cjcenizal by that logic there are no function in this PR that qualify unless we reimplement some core services. All of the asynchronicity in this controller is introduced by the indexPattern.mapper, savedObjects, and esAdmin services, all of which use angular async/http libraries.

If you're suggesting that we write more code that is completely disconnected from angular, that only inter-ops with angular at the last minute using something like $scope.$apply() or wrapping a non angular promise with $q.resolve() then I misunderstood you because I am totally on board with that. Unfortunately we have to adapt a lot of services to not use angular async/http first, or do something (module, app, component) greenfield and use non-angular libraries for everything from async to http.

My stance in this discussion so far was against using angular services in a non-angular context (like an async function() {} that uses the indexPattern.mapper, or esAdmin).

This specific pr is even worse in my opinion, converting any of the helpers to async function(){}s would be creating non-angular contexts between angular services and other angular contexts (Angular controller -> non-angular async function() {} -> angular services). As you stated, this is something we should probably avoid.

All of the asynchronicity in this controller is introduced by the indexPattern.mapper, savedObjects, and esAdmin services, all of which use angular async/http libraries.

Then I guess we can't use $apply. :)

My stance in this discussion so far was against using angular services in a non-angular context (like an async function() {} that uses the indexPattern.mapper, or esAdmin).

I see, sorry, I didn't pick up on that earlier. I thought we were just talking about how to use $apply correctly.

@spalger I think that's an excellent point and something I wasn't considering fully. You're right in that I'm advising to add a layer from angular that needs to go back into angular and that's probably not a good idea unless there is a really good reason. I'm happy with the discussion around this and I 100% agree that we should try and decouple our business logic away from our angular implementation. With that said, I'm fine with the code as-is and appreciate the thoughts!

chrisronline · 2017-05-25T18:53:26Z

...ugins/kibana/public/management/sections/indices/create_index_pattern/create_index_pattern.js

+    this.timeFieldOptionsError = null;
+    this.formValues.timeFieldOption = null;
+    getTimeFieldOptions()
+      .then(results => {


Feels like a good place for object destructuring: .then({ options, error } =>

chrisronline · 2017-05-25T18:55:55Z

...ugins/kibana/public/management/sections/indices/create_index_pattern/create_index_pattern.js

-      this.formValues.nameIsPattern
-      ? this.formValues.nameInterval.name
-      : undefined;
+    const id = name;


Maybe we can just rename the variable in the above destructing assignment instead?

const { name: id,

Yeah, I had it like that at one point, but found it easier to read when all of the values passed to sendCreateIndexPatternRequest() were defined in the same way (rather than using destructuring-rename for one and not others)

chrisronline · 2017-05-25T18:56:55Z

...ugins/kibana/public/management/sections/indices/create_index_pattern/create_index_pattern.js


-    const notExpandable =
-      !this.formValues.expandable && this.canExpandIndices()
+    const timeFieldName = timeFieldOption && timeFieldOption.fieldName;


This variable name makes me think it's a string but the code indicates it's a boolean?

It ends up being the field name or false, which isn't what I want... good catch

chrisronline · 2017-05-25T18:57:46Z

...ugins/kibana/public/management/sections/indices/create_index_pattern/create_index_pattern.js

+    // Only event-time-based index patterns set an intervalName.
+    const intervalName = (nameIsPattern && nameInterval)
+      ? nameInterval.name
+      : undefined;


Why undefined instead of false?

In my eyes false represents "no", and undefined/null represent "no value" (with slightly different semantics depending on who you ask).

This is trying to represent "no intervalName", so I don't think false is appropriate.

chrisronline · 2017-05-25T19:01:37Z

...ugins/kibana/public/management/sections/indices/create_index_pattern/create_index_pattern.js

-  const updateFieldListAndSetTimeField = (results, timeFieldName) => {
-    updateFieldList(results);
+  const pickDefaultTimeFieldOption = () => {
+    const noOptions = this.timeFieldOptions.length === 0;


I think we should try and use pure functions as much as possible, so in this case, can we simply use an argument timeFieldOptions instead of relying on this.?

This is another thing I tried and reverted, mostly because of the ambiguity caused by the existence of multiple timeFieldOptions in scope, and it's strong dependence on the structure of the shape of timeFieldOptions. If we moved this function out of the controller module along with the rest of the timeFieldOptions methods I don't think it would be ambiguous, but that seems like a much larger change than necessary.

That said, this function intentionally doesn't produce side effects so that it's pure by some definitions.

I'd actually be in favor of that decision (to move it outside of the controller). Yes it doesn't produce side effects and that might be the more important aspect of pure functions but it still does rely on scoped state when it doesn't need to. I have no desire to hold up the review for this particular topic, as I'm just trying to understand how everyone feels about this

strong dependence on the structure of the shape of timeFieldOptions

By this I mean to say that every time this.timeFieldOptions is modified in the future this function and its logic needs to be considered. Unfortunately, passing the timeFieldOptions in as an argument doesn't change that but, IMO, creates an illusion of decoupling.

Example:
Say I wanted to add a property to each timeFieldOption I would probably track down where they are created, then where they are stored, and then where they are used. In order to know that this function uses the this.timeFieldOptions array I would first have to find the location that reads this.timeFieldOption and then passes it to this function because pickDefaultTimeFieldOption()'s dependence on this.timeFieldOptions is essentially hidden. If instead I leave this function accessing this.timeFieldOption directly it's obvious that it is dependent.

Yea that makes sense, and I don't think there is any decoupling happening here. There is and will always be a hard dependence, but I don't think the changes I'm suggesting make that coupling any less obvious, as when you're doing a search for this.timeFieldOptions, you'd see it passed into the function.

But as a pure function, you can:

Easily unit test as you're not dependent on any scoped state, or instance data to setup or tear down

Easily refactor the internals with zero risk of side effects

Easily migrate to a new file

Easily refactor/rename outside scoped/instanced variables without worrying about them affecting the internals of the function (you just might need to update the name of the provided parameter)

Easily document/explain the purpose of the function in the context of input and output

IMO, it seems like an easy win to strive for pure as much as possible. It's very possible that I'm not seeing a part of this that makes it harder or undesired and open to listening to other perspectives but the more codebases that I've worked in and the longer I've been writing code, the more I'm thinking that pure functions are a win/win across the board.

I agree with @chrisronline's reasoning here. I did some similar extraction as part of https://github.com/elastic/kibana/pull/11285/files#diff-053c72f0b419832d5edb39bd81422e06R1 and it made the controller logic easier to follow because it was higher level.

I'm okay with this not going in right now. I think I'm going to open a discussion issue about this in general to get some general thoughts and best practices that others have learned. I do think it's an important conversation but it doesn't have to happen in this PR

chrisronline · 2017-05-25T19:04:22Z

...ugins/kibana/public/management/sections/indices/create_index_pattern/create_index_pattern.js

+      });
+    })
+    .then(
+      fields => {


Is there a reason this is on a newline and not:

}) .then(fields =>

?

So it lines up with the errorback below. With this version of the implementation it's not necessary to use a single .then(), so I'll switch it to .then().catch()

chrisronline · 2017-05-25T19:06:05Z

...ugins/kibana/public/management/sections/indices/create_index_pattern/create_index_pattern.js

+            {
+              display: $translate.instant('KIBANA-NO_DATE_FIELD_DESIRED')
+            },
+            ...dateFields.map(field => ({


spalger · 2017-05-25T19:54:19Z

@chrisronline @cjcenizal thanks for all the awesome feedback, ready for round three! 💃

cjcenizal

🏄 LGTM! I think it's worth exploring @chrisronline's suggestions about pure-function-extraction and async/await + $apply.

chrisronline

LGTM!

* [indexPatterns/create] timeFields -> timeFieldOptions In #11409 we added two new possible options to the "Time Filter field name" drop down. These options display a message that "there are no time fields" or "I don't want to use the time filter". These new options were added to the `controller.timeFields` array, which is where the other options were, but they don't actually represent time fields. This change updates `controller.timeFields` to instead be a list of options, specifically created to populate the select box in the view. They are now stored at `controller.timeFieldOptions` and each option has a `display` property (which will be rendered in the view) and optionally a `fieldName` property. The selected option, `controller.formValues.timeFieldOption` is checked for a `fieldName` property when the Index Pattern is created. * [indexPatterns/create] populate `this.timeFieldOptions` * [indexPatterns/create] unify fetching and errors for timeFieldOptions * [indexPatterns/create] refreshFieldList() -> refreshTimeFieldOptions() * [indexPatterns/create] unify "No time fields available" messages * [indexPatterns/create] disable the timeField select when loading * [indexPatterns/create] only require a time field is there are options Doing this prevents the time field from rendering as invalid when there is no matching index pattern (which has it's own error display) and when the fields are loading * [indexPatterns/create] isolate side-effects * [indexPatterns/create] let refreshTimeFieldOptions() control model * [indexPatterns/create] attempt to make default field selection clearer * [indexPatterns/create] explain why we set timeFieldOption * [indexPatterns/create] make timeFieldName `undefined` when not set * [indexPatterns/create] Don't pass two args to .then() unless needed * 💅 (cherry picked from commit 801549c)

* [indexPatterns/create] timeFields -> timeFieldOptions In elastic#11409 we added two new possible options to the "Time Filter field name" drop down. These options display a message that "there are no time fields" or "I don't want to use the time filter". These new options were added to the `controller.timeFields` array, which is where the other options were, but they don't actually represent time fields. This change updates `controller.timeFields` to instead be a list of options, specifically created to populate the select box in the view. They are now stored at `controller.timeFieldOptions` and each option has a `display` property (which will be rendered in the view) and optionally a `fieldName` property. The selected option, `controller.formValues.timeFieldOption` is checked for a `fieldName` property when the Index Pattern is created. * [indexPatterns/create] populate `this.timeFieldOptions` * [indexPatterns/create] unify fetching and errors for timeFieldOptions * [indexPatterns/create] refreshFieldList() -> refreshTimeFieldOptions() * [indexPatterns/create] unify "No time fields available" messages * [indexPatterns/create] disable the timeField select when loading * [indexPatterns/create] only require a time field is there are options Doing this prevents the time field from rendering as invalid when there is no matching index pattern (which has it's own error display) and when the fields are loading * [indexPatterns/create] isolate side-effects * [indexPatterns/create] let refreshTimeFieldOptions() control model * [indexPatterns/create] attempt to make default field selection clearer * [indexPatterns/create] explain why we set timeFieldOption * [indexPatterns/create] make timeFieldName `undefined` when not set * [indexPatterns/create] Don't pass two args to .then() unless needed * 💅

spalger added :Management review v5.5.0 v6.0.0 labels May 25, 2017

spalger requested review from ycombinator and cjcenizal May 25, 2017 01:00

Merge branch 'master' of github.com:elastic/kibana into index-pattern…

f8caf83

…s/time-field-options

cjcenizal suggested changes May 25, 2017

View reviewed changes

[indexPatterns/create] populate this.timeFieldOptions

4cb39d3

spalger force-pushed the index-patterns/time-field-options branch 4 times, most recently from f3f827d to df95118 Compare May 25, 2017 15:45

[indexPatterns/create] unify fetching and errors for timeFieldOptions

c3c3829

spalger force-pushed the index-patterns/time-field-options branch 8 times, most recently from b4d98a3 to 887e451 Compare May 25, 2017 17:18

spalger added 3 commits May 25, 2017 10:24

[indexPatterns/create] refreshFieldList() -> refreshTimeFieldOptions()

ca93050

[indexPatterns/create] unify "No time fields available" messages

1cf49c0

[indexPatterns/create] disable the timeField select when loading

b3f7d83

spalger force-pushed the index-patterns/time-field-options branch 3 times, most recently from ffcaf46 to e3eb3bc Compare May 25, 2017 18:03

spalger changed the title ~~[indexPatterns/create] timeFields -> timeFieldOptions~~ [indexPatterns/create] refactor time field options May 25, 2017

spalger added 2 commits May 25, 2017 11:07

[indexPatterns/create] only require a time field is there are options

8ea9813

Doing this prevents the time field from rendering as invalid when there is no matching index pattern (which has it's own error display) and when the fields are loading

[indexPatterns/create] isolate side-effects

8d1df86

spalger force-pushed the index-patterns/time-field-options branch from e3eb3bc to 8d1df86 Compare May 25, 2017 18:10

[indexPatterns/create] let refreshTimeFieldOptions() control model

b8a7f4f

spalger mentioned this pull request May 25, 2017

[indexPatterns/create] dedupe refreshTimeFieldOptions() #12007

Merged

ycombinator requested review from chrisronline and removed request for ycombinator May 25, 2017 18:32

cjcenizal suggested changes May 25, 2017

View reviewed changes

spalger added 2 commits May 25, 2017 11:54

[indexPatterns/create] attempt to make default field selection clearer

6ad5ff8

[indexPatterns/create] explain why we set timeFieldOption

cebb6a6

chrisronline suggested changes May 25, 2017

View reviewed changes

spalger added 3 commits May 25, 2017 12:45

[indexPatterns/create] make timeFieldName undefined when not set

7e33e0f

[indexPatterns/create] Don't pass two args to .then() unless needed

4eab34b

💅

54a80f6

cjcenizal approved these changes May 25, 2017

View reviewed changes

spalger mentioned this pull request May 25, 2017

[indexPatterns/create] disable time-based options when needed #12011

Merged

chrisronline approved these changes May 26, 2017

View reviewed changes

spalger merged commit 801549c into elastic:master May 26, 2017

chrisronline mentioned this pull request Jun 13, 2017

Start of a react style guide #12240

Merged

spalger deleted the index-patterns/time-field-options branch October 18, 2019 17:43

[indexPatterns/create] refactor time field options #11996

[indexPatterns/create] refactor time field options #11996

Conversation

spalger commented May 25, 2017 • edited Loading

cjcenizal left a comment

Choose a reason for hiding this comment

cjcenizal May 25, 2017 • edited Loading

Choose a reason for hiding this comment

spalger commented May 25, 2017

cjcenizal 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

spalger commented May 25, 2017

chrisronline left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger May 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger May 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal May 25, 2017 • edited by spalger 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger May 25, 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

spalger commented May 25, 2017

cjcenizal left a comment

Choose a reason for hiding this comment

chrisronline left a comment

Choose a reason for hiding this comment

spalger commented May 25, 2017 •

edited

Loading

cjcenizal May 25, 2017 •

edited

Loading

spalger May 25, 2017 •

edited

Loading

spalger May 25, 2017 •

edited

Loading

cjcenizal May 25, 2017 •

edited by spalger

Loading

spalger May 25, 2017 •

edited

Loading