Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: chart state and series functions cleanup #989

Merged
merged 27 commits into from
Jan 25, 2021

Conversation

monfera
Copy link
Contributor

@monfera monfera commented Jan 21, 2021

Summary

Mild refactoring that leaves logic and types in place:

  • fixes the handling of zero non-global chart types
  • fixes a sorting issue with non-finite returns
  • removes some branching and dries up conditionals
  • removes avoidable variables, lets and conditional mutation
  • attempts to improve the readability and "flatness" of some logic, converting some triangular bits to linear and rectangular shape
  • regularizes returns into one or just very few, easy to spot places
  • removes some type related branching that used to (or should have, sans TS limitations) narrowed to a singular type or never
  • added a heterogeneous small multiples test case we looked at together
  • removes IDE-generated JSDocs @params encountered

No functional or API change. Precedes further work on non-cartesian small multiples.

Checklist

Delete any items that are not applicable to this PR.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
doodspav doodspav
@monfera monfera added the chore label Jan 21, 2021
@monfera monfera requested a review from nickofthyme January 21, 2021 18:03
@monfera monfera changed the title [refactor] minor update to some chart state and series functions refactor: minor update to some chart state and series functions Jan 21, 2021
const multipleYAccessors = spec && spec.yAccessors.length > 1;
const nameKeys = multipleYAccessors ? seriesIdentifier.seriesKeys : seriesIdentifier.seriesKeys.slice(0, -1);
const nonZeroLength = nameKeys.length > 0;

Copy link
Contributor Author

@monfera monfera Jan 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The block below (return nonZeroLength...) could be extracted into a separate function, though not much benefit right now. And flat, linear ternary chaining for the inherently fall-through logic, we might need to chat about it 😃

@monfera monfera requested a review from markov00 January 21, 2021 18:14
@monfera monfera requested a review from rshen91 January 21, 2021 18:14
Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good for me, thanks for taking time to clean this up, in particular the state reducer,
I've just set a comment to rotate the added chart horizontally that looks a bit better IMHO

src/chart_types/xy_chart/utils/series.ts Show resolved Hide resolved
stories/small_multiples/6_heterogeneous_cartesians.tsx Outdated Show resolved Hide resolved
Copy link
Collaborator

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't agree with all the if statements being converted to nested ternaries, in some cases they a cleaner but in many cases, the compact logic is hard to follow. I think this should be a preference thing when using nesting ternaries is required.

@@ -17,28 +17,9 @@
* under the License.
*/

/*
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏼

src/chart_types/xy_chart/utils/series.ts Show resolved Hide resolved
src/chart_types/xy_chart/utils/series.ts Show resolved Hide resolved
Comment on lines +547 to +552
const customLabel =
typeof spec?.name === 'function'
? spec.name(seriesIdentifier, isTooltip)
: typeof spec?.name === 'object' // extract booleans once https://github.com/microsoft/TypeScript/issues/12184 is fixed
? getSeriesNameFromOptions(spec.name, seriesIdentifier, spec.name.delimiter ?? SERIES_DELIMITER)
: null;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems harder to read for me TBH, not sure why the nested ternary lines are not indented.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's not nesting b/c it's a linear fallthrough case, ie. no structural nesting applies, more like a case sequence in a switch. Would work with indented formatting, would still preserve ability to linearly read through

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a ternary inside another ternary, that's nested no?

As far as the indenting, this is what I'd like to see that delineates the exprIfTrue and exprIfTrue code blocks.

const customLabel =
    typeof spec?.name === 'function'
      ? spec.name(seriesIdentifier, isTooltip)
      : typeof spec?.name === 'object' // extract booleans once https://github.com/microsoft/TypeScript/issues/12184 is fixed
        ? getSeriesNameFromOptions(spec.name, seriesIdentifier, spec.name.delimiter ?? SERIES_DELIMITER)
        : null;

Copy link
Contributor Author

@monfera monfera Jan 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This formatting would be OK too, maybe in some formatting PR we can test drive how the rule would shape the ternaries here and elsewhere.
You are right, the syntax AST is a right-deep tree which is conceptually still linear (the reason it was born), as in, you could substitute it with a reduce, or switch/case alternative, or it can be read as

Condition1
Result 1
Condition2
Result2
Condition3
Result3
...

Especially with code I didn't write, I find it way easier to linearly step through, "OK this condition still not true, let's look whether the next applies" compared to needing to simulate ascending and descending into (semantically, not just AST-wise) nested ifs. So this is what I meant by rectangular code, linear readability. I know I'm in trouble when the left edge of the code is meandering, I might have some weaknesses there, usually, it takes less time to simplify data flow and control flow paths and making the code shorter (all of which I think also happened) than to eyeball it long enough to grok what's going on.

Languages:

JS doesn't offer a true cond, and the equivalent switch(true) pattern is not only a yoda condition but also a bit unwieldy (b/c our linter requires curlies for even single-row returns), though often better than nested if. A reduce with the options would work too, but it'd eagerly evaluate never taken options, expensive and would need a lot of defensive optional chaining etc.
The best support in this case is provided by those that have pattern matching constructs. Eg, a Fibonacci in haskell would be

fib n | n == 0 = 1
      | n == 1 = 1
      | n >= 2 = fib (n-1) + fib (n-2)

again, linear/rectangular code, easy to read top to bottom, and a single pass reading gives a good picture, no need to ascend and descend into conditional valleys. JS don't have no such pattern matching...

Bigger picture:

Both code (internal view, our DX) and API (external view, Kibana DX) benefit from a strive toward simplicity. One way for achieving it is to reduce the number of conditional paths, nesting variety and one-off handling. It's always straightforward to add to the code; "on this condition X let's do something different before/after/instead of the current thing", which is how code bases become tangled.

So my feel is that the real solution is not to turn into ternary all such bushy if trees, though it has some positives too here, and also, not to introduce code level constraints.

Instead, we could be be a bit slower to add features; let's brainstorm if

  • we actually need a feature (eg. this function is made this complex in part by the null return of the callback we discussed today; meanwhile nobody uses it, and even our test cases don't lock in the promising yet speculative flexibility)
  • can it be achieved without added complexity, or how can we minimize added complexity
  • can we even remove branchy code while solving a need by going a bit more general; eg. avoiding singleton cases

That's the drawback with broadening types into union types (discriminated or not), and/or optional props, and the error prone falsey and fallthrough cases that require mental juggling and exponentially growing conditional branches like | null | undefined. This is why I feel it was good to chat about lending the user overriding and other config options in ways that retain as much homogeneity as possible, and type clarity helps the user too, fewer types, tighter doc etc. This is all I'm learning along the way so quote me on this when running into my subpar code, past or future 😸

We talked about eventually farming out a humane layer for somewhat hairy rules, defaults and fallbacks, so our core isn't penetrated by API use brevity or other architecture wise superficially manageable reasons; ie. core charting, wrapped around with defaults etc. driven by best practices, folks' expectations, customs etc. We had a chat with @markov00 today about an approach toward layered, priority/fallback driven configuration, which doesn't bake in what the user can and cannot override along some preconceived directions. So either of these options could help avoid expoding conditionals.

So ultimately I agree, and work toward not needing this many layers of "pattern matching", ternaries or not. Future PRs from all of us FTW!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks as always for the thoughtful reply. I just don't want to get to a point where we are forming super complex ternaries to remove complex nested conditionals. Replacing some makes sense while others would be hard to understand, but yes reducing the bushyness would be great.

As for the null option and other non-used functionality. I think we could go through the code sometime and maybe identify features in the code that are any of the following:

  • Not used and does not have a practicable use case.
  • Features that are not used but have a good use case. (documentation is to blame)
  • Features or options that have multiple (>2) options to use but only a simple and complex option would suffice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @nickofthyme!

Great idea for hunting for disused functionality (esp. that's not foreseen to be used; maybe this null or some alternative would have great use). Maybe we could look out for the replaceability of optional or nullable/undefinable properties too, at least for alternatives discussion. Though both take a good bunch of time.

I agree the ternary chaining is less than optimal, there's room for improving on it, hopefully by simplification rather than eg. if/return or let/if/assign chaining or nesting the next time one of us touches that part of the code, so it's likely not for eternity.

src/chart_types/xy_chart/utils/series.ts Show resolved Hide resolved
src/state/actions/colors.ts Outdated Show resolved Hide resolved
src/state/chart_state.ts Show resolved Hide resolved
src/utils/common.ts Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Jan 21, 2021

Codecov Report

Merging #989 (17e1356) into master (d14de01) will increase coverage by 0.48%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #989      +/-   ##
==========================================
+ Coverage   70.87%   71.36%   +0.48%     
==========================================
  Files         346      362      +16     
  Lines       11028    11314     +286     
  Branches     2419     2448      +29     
==========================================
+ Hits         7816     8074     +258     
- Misses       3199     3221      +22     
- Partials       13       19       +6     
Flag Coverage Δ
unittests 70.85% <100.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/chart_types/xy_chart/utils/series.ts 96.05% <100.00%> (+0.28%) ⬆️
src/state/chart_state.ts 89.74% <100.00%> (+1.83%) ⬆️
src/utils/common.ts 90.41% <100.00%> (+0.06%) ⬆️
src/mocks/store/index.ts 100.00% <0.00%> (ø)
src/mocks/store/store.ts 92.59% <0.00%> (ø)
src/mocks/specs/index.ts 100.00% <0.00%> (ø)
src/mocks/geometries.ts 92.10% <0.00%> (ø)
src/mocks/series/series.ts 90.00% <0.00%> (ø)
src/mocks/index.ts 100.00% <0.00%> (ø)
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d14de01...17e1356. Read the comment docs.

@monfera monfera requested a review from nickofthyme January 21, 2021 20:49
@monfera monfera changed the title refactor: minor update to some chart state and series functions fix: minor update to some chart state and series functions Jan 21, 2021
@monfera
Copy link
Contributor Author

monfera commented Jan 21, 2021

Re-titled to fix as it fixes two low likelihood runtime issues (zero chartTypes -> undefined return; Infinity and NaNs in sort function) and improves types (fewer asserts, removal of a never taken code path)

Copy link
Collaborator

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Latest changes LGTM. Thanks for the comments.

@nickofthyme nickofthyme changed the title fix: minor update to some chart state and series functions fix: chart state and series functions cleanup Jan 25, 2021
@nickofthyme
Copy link
Collaborator

@monfera I think fix is also good, I think a more concise title is fine. Feel free to change it back if you like.

@monfera monfera merged commit 944ac6c into elastic:master Jan 25, 2021
github-actions bot pushed a commit that referenced this pull request Jan 30, 2021
# [24.5.0](v24.4.0...v24.5.0) (2021-01-30)

### Bug Fixes

* add theme min radius to point shape ([#996](#996)) ([eb37175](eb37175))
* align tooltip z-index to EUI tooltip z-index ([#931](#931)) ([ffd626b](ffd626b))
* chart state and series functions cleanup ([#989](#989)) ([944ac6c](944ac6c))
* create unique ids for dot icons ([#971](#971)) ([e1ce768](e1ce768))
* external tooltip legend extra value sync ([#993](#993)) ([13ad05a](13ad05a))
* **legend:** disable focus and keyboard navigation for legend in partition ch… ([#952](#952)) ([03bd2f7](03bd2f7))
* **legend:** hierarchical legend order should follow the tree paths ([#947](#947)) ([f9218ad](f9218ad)), closes [#944](#944)
* **legend:** remove ids for circles ([#973](#973)) ([b3f4f90](b3f4f90))

### Features

* **cursor:** improve theme styling for crosshair ([#980](#980)) ([6c4dafd](6c4dafd))
* **legend:**  display pie chart legend extra ([#939](#939)) ([d14de01](d14de01))
* **legend:** add keyboard navigation ([#880](#880)) ([87c227d](87c227d))
* **partition:** Flame and icicle chart ([#965](#965)) ([3df73d0](3df73d0))
* **partition:** legend hover options ([#978](#978)) ([f810d94](f810d94))
* **xy:** support multiple point shapes on line, area and bubble charts ([#988](#988)) ([1392b7d](1392b7d))
@markov00
Copy link
Member

🎉 This PR is included in version 24.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Jan 30, 2021
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
# [24.5.0](elastic/elastic-charts@v24.4.0...v24.5.0) (2021-01-30)

### Bug Fixes

* add theme min radius to point shape ([opensearch-project#996](elastic/elastic-charts#996)) ([98089a9](elastic/elastic-charts@98089a9))
* align tooltip z-index to EUI tooltip z-index ([opensearch-project#931](elastic/elastic-charts#931)) ([f7f1f6f](elastic/elastic-charts@f7f1f6f))
* chart state and series functions cleanup ([opensearch-project#989](elastic/elastic-charts#989)) ([42a7af0](elastic/elastic-charts@42a7af0))
* create unique ids for dot icons ([opensearch-project#971](elastic/elastic-charts#971)) ([0b3e00f](elastic/elastic-charts@0b3e00f))
* external tooltip legend extra value sync ([opensearch-project#993](elastic/elastic-charts#993)) ([7e1096e](elastic/elastic-charts@7e1096e))
* **legend:** disable focus and keyboard navigation for legend in partition ch… ([opensearch-project#952](elastic/elastic-charts#952)) ([dfff3e2](elastic/elastic-charts@dfff3e2))
* **legend:** hierarchical legend order should follow the tree paths ([opensearch-project#947](elastic/elastic-charts#947)) ([7b70186](elastic/elastic-charts@7b70186)), closes [opensearch-project#944](elastic/elastic-charts#944)
* **legend:** remove ids for circles ([opensearch-project#973](elastic/elastic-charts#973)) ([ed98481](elastic/elastic-charts@ed98481))

### Features

* **cursor:** improve theme styling for crosshair ([opensearch-project#980](elastic/elastic-charts#980)) ([0248ad6](elastic/elastic-charts@0248ad6))
* **legend:**  display pie chart legend extra ([opensearch-project#939](elastic/elastic-charts#939)) ([672a4df](elastic/elastic-charts@672a4df))
* **legend:** add keyboard navigation ([opensearch-project#880](elastic/elastic-charts#880)) ([b471a94](elastic/elastic-charts@b471a94))
* **partition:** Flame and icicle chart ([opensearch-project#965](elastic/elastic-charts#965)) ([9e8b1f7](elastic/elastic-charts@9e8b1f7))
* **partition:** legend hover options ([opensearch-project#978](elastic/elastic-charts#978)) ([acd1339](elastic/elastic-charts@acd1339))
* **xy:** support multiple point shapes on line, area and bubble charts ([opensearch-project#988](elastic/elastic-charts#988)) ([4f23b4f](elastic/elastic-charts@4f23b4f))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore released Issue released publicly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants