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

[Feature Anywhere] Fix visibleVisLayers ser/deser #3758

Merged

Conversation

ohltyler
Copy link
Member

Description

The change to using a Map for persisting visible VisLayers in the vega spec config had a bug where during the string serialization/deserialization (necessary for passing through the expression functions), the Map obj could not be properly stringified which was leading to an empty Map when parsing in the VegaParser. This was causing no VisLayers to be able to render.

This fixes that by converting the Map => array when serializing, and converting back to a Map when deserializing in the parser.

Note another option could have been using a basic JS obj {} for persisting this, but that would limit us in not being able to use the VisLayerTypes enum as keys, and would need other conversion as well.

Also hardens a helper method by handling a case where events is undefined or empty, + a regression test for that.

Issues Resolved

[List any issues this PR will resolve]

Check List

  • Commits are signed per the DCO using --signoff

Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
@ohltyler ohltyler requested a review from a team as a code owner March 31, 2023 17:16
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
@codecov-commenter
Copy link

codecov-commenter commented Mar 31, 2023

Codecov Report

Merging #3758 (c15af26) into feature/feature-anywhere (8a2cc69) will increase coverage by 0.00%.
The diff coverage is 50.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@                    Coverage Diff                    @@
##           feature/feature-anywhere    #3758   +/-   ##
=========================================================
  Coverage                     66.50%   66.51%           
=========================================================
  Files                          3225     3225           
  Lines                         61980    61982    +2     
  Branches                       9542     9543    +1     
=========================================================
+ Hits                          41221    41225    +4     
+ Misses                        18473    18471    -2     
  Partials                       2286     2286           
Flag Coverage Δ
Linux 66.45% <50.00%> (+<0.01%) ⬆️
Windows 66.45% <50.00%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
...ins/vis_type_vega/public/data_model/vega_parser.ts 77.23% <0.00%> (-0.64%) ⬇️
src/plugins/vis_augmenter/public/test_constants.ts 100.00% <100.00%> (ø)
src/plugins/vis_augmenter/public/vega/helpers.ts 97.95% <100.00%> (-0.03%) ⬇️

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Copy link
Member

@ananzh ananzh left a comment

Choose a reason for hiding this comment

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

Nice!

@ananzh ananzh merged commit de61732 into opensearch-project:feature/feature-anywhere Mar 31, 2023
@ohltyler ohltyler deleted the map-bugfix branch March 31, 2023 20:19
@kavilla kavilla added v2.9.0 and removed v2.8.0 labels Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants