grafana / scenes

Build Grafana dashboards directly in your Grafana app plugins.
https://grafana.com/developers/scenes
Apache License 2.0
141 stars 21 forks source link

VizPanel: Clear options value when the option is undefined #801

Closed ivanortegaalba closed 4 months ago

ivanortegaalba commented 4 months ago

Fixes #89270

Problem _.mergeWith ignores undefined values from the source object. When clearing a VizPanel option, we set it to undefined so it was not working.

To be able to set an option as undefined, we have to treat that case in the customizer cb passed to _.mergeWith.

Before After
📦 Published PR as canary version: 5.2.1--canary.801.9610977729.0
:sparkles: Test out this PR locally via: ```bash npm install @grafana/scenes-react@5.2.1--canary.801.9610977729.0 npm install @grafana/scenes@5.2.1--canary.801.9610977729.0 # or yarn add @grafana/scenes-react@5.2.1--canary.801.9610977729.0 yarn add @grafana/scenes@5.2.1--canary.801.9610977729.0 ```
torkelo commented 4 months ago

I thought we set replace=true when changing options? We do that when we change field options

ivanortegaalba commented 4 months ago

I thought we set replace=true when changing options? We do that when we change field options

If I'm not wrong, we're not https://github.com/grafana/grafana/blob/main/public/app/features/dashboard/components/PanelEditor/getVisualizationOptions.tsx#L223

Anyways, the merge for options, even if it doesn't replace, should be considered undefined as a valid value. Otherwise, we will be always forced to replace if we want to remove the value of a property from an object.

ivanortegaalba commented 4 months ago

Great work, @ivanortegaalba 🥇 , I did a local test and indeed this fixes the issue. I think the approach makes sense, as you said, we don't want to replace the whole object, but partial options.

Approving the PR, however, it seems a file was added to the PR by error?

filename.txt

Ups! OMG, sorry! 🤦 Removed!

grafanabot commented 4 months ago

:rocket: PR was released in v5.2.1 :rocket: