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: react/redux issue with specParser #723

Merged
merged 5 commits into from
Jun 29, 2020

Conversation

nickofthyme
Copy link
Collaborator

@nickofthyme nickofthyme commented Jun 26, 2020

Summary

Fixes #720

  • update react versions to ^16.3.0
  • add redux middleware for debugging
  • refactor spec parsing reducers

- update react versions to ^16.3.0
- add redux middleware for debugging
- add broken code example to playground
@nickofthyme nickofthyme requested a review from markov00 June 26, 2020 00:42
@markov00
Copy link
Member

markov00 commented Jun 26, 2020

One possible fix is the following:
removing the injected.specParsing(); call on the SpecsParserComponent this is what is causing the issue in that case.
We have also to update the reducer to handle the parsing in a different way:

  • removing the SPEC_PARSING reducer
  • on the SPEC_PARSED reducer we should update the state.specs taking care of a possible missing Setting component like:
specs: {
              ...state.specs,
              [DEFAULT_SETTINGS_SPEC.id]: state.specs[DEFAULT_SETTINGS_SPEC.id] ?? DEFAULT_SETTINGS_SPEC,
            },
  • on the UPSERT_SPEC action reducer we should change the state applying the SPEC_PARSING flags like:
specsInitialized: false,
chartRendered: false,

This should solve the issue without causing any major problem on our current "parsing" mechanism

- remove specParsing action and reducer
- move default settings spec into parsed reducer
- add tests for spec swapping with different ids
- fix unit tests based on changes
@codecov-commenter
Copy link

Codecov Report

Merging #723 into master will decrease coverage by 0.57%.
The diff coverage is 73.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #723      +/-   ##
==========================================
- Coverage   74.80%   74.23%   -0.58%     
==========================================
  Files         265      267       +2     
  Lines        8541     8588      +47     
  Branches     1714     1732      +18     
==========================================
- Hits         6389     6375      -14     
- Misses       2099     2161      +62     
+ Partials       53       52       -1     
Impacted Files Coverage Δ
src/chart_types/goal_chart/state/chart_state.tsx 59.18% <0.00%> (ø)
.../chart_types/partition_chart/state/chart_state.tsx 74.50% <0.00%> (ø)
src/components/portal/utils.ts 21.42% <0.00%> (-78.58%) ⬇️
src/specs/constants.ts 100.00% <ø> (ø)
src/state/actions/specs.ts 100.00% <ø> (+6.66%) ⬆️
src/state/spec_factory.ts 96.15% <ø> (+3.84%) ⬆️
src/components/portal/tooltip_portal.tsx 12.30% <9.09%> (-53.32%) ⬇️
src/state/chart_state.ts 87.30% <33.33%> (+0.20%) ⬆️
...state/selectors/get_internal_is_tooltip_visible.ts 75.00% <50.00%> (ø)
src/components/tooltip/get_tooltip_settings.ts 57.14% <57.14%> (ø)
... and 26 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 88123f5...3606512. Read the comment docs.

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.

LGTM

@nickofthyme nickofthyme merged commit f9c29ec into elastic:master Jun 29, 2020
@nickofthyme nickofthyme deleted the fix-redux-error branch June 29, 2020 15:48
markov00 pushed a commit that referenced this pull request Jun 29, 2020
## [19.6.2](v19.6.1...v19.6.2) (2020-06-29)

### Bug Fixes

* react/redux issue with specParser ([#723](#723)) ([f9c29ec](f9c29ec)), closes [#720](#720)
@markov00
Copy link
Member

🎉 This PR is included in version 19.6.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Jun 29, 2020
markov00 added a commit to markov00/elastic-charts that referenced this pull request Jul 6, 2020
This commit reset the chart state to not initialized when removing a spec. After elastic#723 PR that
reduced the number of steps on the state machine when parsing, the removeSpec action wasn't
accounted on that refactoring causing side effects when removing/switching a spec on the chart
configuration due to the state being in an wrong status.

fix elastic#738
markov00 added a commit that referenced this pull request Jul 6, 2020
This commit reset the chart state to not initialized when removing a spec. After #723 PR that
reduced the number of steps on the state machine when parsing, the removeSpec action wasn't
accounted on that refactoring causing side effects when removing/switching a spec on the chart
configuration due to the state being in an wrong status.

fix #738
markov00 pushed a commit that referenced this pull request Jul 6, 2020
# [19.8.0](v19.7.0...v19.8.0) (2020-07-06)

### Bug Fixes

* set uninitialized state when removeSpec action is called ([#739](#739)) ([35b8caf](35b8caf)), closes [#723](#723) [#738](#738)

### Features

* **annotation:** enable marker positioning on LineAnnotation ([#737](#737)) ([ab5e413](ab5e413)), closes [#701](#701)
* add custom annotation tooltip ([#727](#727)) ([435c67c](435c67c))
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Issue released publicly
Projects
None yet
3 participants