search

grafana / scenes

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

SafeSerializableSceneObject: Wrap only for supported Grafana version #854

Closed dprokop closed 1 month ago

dprokop commented 1 month ago

This PR brings backwards compatibility for scopedVars.__sceneObject and SafeSerializableSceneObject. (ref https://github.com/grafana/scenes/pull/828, https://github.com/grafana/scenes/issues/851)

With this change, Scenes apps using 5.8+ will be able to run smoothly in Grafana versions that do not contain https://github.com/grafana/grafana/pull/90833.

For versions that will have this included, that is :

scopedVars.__sceneObject WILL use SafeSerializableSceneObject. For versions other than mentioned above, scopedVars.__sceneObject will be just an old good plan ScopedVar.

📦 Published PR as canary version: 5.10.0--canary.854.10453539660.0
:sparkles: Test out this PR locally via: ```bash npm install @grafana/scenes-react@5.10.0--canary.854.10453539660.0 npm install @grafana/scenes@5.10.0--canary.854.10453539660.0 # or yarn add @grafana/scenes-react@5.10.0--canary.854.10453539660.0 yarn add @grafana/scenes@5.10.0--canary.854.10453539660.0 ```

Release notes

Brings a fix for variables interpolation bug when apps using scenes 5.6.0+ were run in Grafana version lower than 11.2.0, 11.1.2, 11.0.4, 10.4.8.

dprokop commented 1 month ago

It feels a bit backward to have the library change behavior depending on the version of what is using the library, but I can see this being the most pragmatic solution.

Agree, but this feels best what we can do with a change like this.

I think the code looks good, and the test coverage is there. I guess testing this manually on the grafana versions affected could be done when we're bumping scenes to this version.

You can already test it with the canary release. I don't want to merge this until reviewers will run an app with this version agains versions we are building compatibility for.

torkelo commented 1 month ago

@dprokop any reason this is not merged?

dprokop commented 1 month ago

@torkelo - yeah, didn't get anyone's eyes for testing this yet. Asked the team to help with getting this landed as soon as possible.

torkelo commented 1 month ago

@dprokop ah, I see. no worries, just going through PRs, wanted to make sure it was not forgotten

dprokop commented 1 month ago

@axelavargas - thanks for running this! I've made a version ranges mistake - 11.0.3 and 10.4.7 were security releases and those did not include the corresponding backports. I've update the version ranges to reflect the upcoming patch releases. And also updated the description of this PR to reflect the updated ranges.

grafanabot commented 1 month ago

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