-
Notifications
You must be signed in to change notification settings - Fork 15
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
PlainReact: Expose scene features through contexts and hooks and normal react components #734
base: main
Are you sure you want to change the base?
Conversation
export const plainGraph = RVisualizationBuilders.timeseries().setCustomFieldConfig('fillOpacity', 6).build(); | ||
|
||
export const timeSeriesBars = RVisualizationBuilders.timeseries() | ||
.setCustomFieldConfig('drawStyle', GraphDrawStyle.Bars) | ||
.setCustomFieldConfig('fillOpacity', 6) | ||
.build(); | ||
|
||
export const graphWithGrapdientColor = RVisualizationBuilders.timeseries() | ||
.setCustomFieldConfig('fillOpacity', 10) | ||
.setCustomFieldConfig('lineWidth', 3) | ||
.setCustomFieldConfig('gradientMode', GraphGradientMode.Scheme) | ||
.setColor({ mode: FieldColorModeId.ContinuousGrYlRd }) | ||
.build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really really like this separation between panel and visualization, here it's super easy to just define visualization definitions that you can share / reuse in your scene app.
This separation was something I tried really hard to do with VizPanel (breaking it up into two abstractions), but failed to figure out how, as whatever renders PanelChrome needs data, and for data we need the visualization
But with RVizPanel the separation between panel and visualization is very clean. We can actually do the same thing for VizPanel, just need to package pluginId, pluginVersion, options & fieldConfig into one object like we do for RVizPanel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing it's missing is extending a defined visualization with new overrides. Normally the overrides (like data links and naming overrides) will be data / panel specific.
so we could have something like.
const vizWithOverrides = RVisualizationBuilders.extend(graphWithGrapdientColor)
.setOverrides((b) =>
b.matchFieldsWithName('Library').overrideLinks([
{
title: 'Go to library details',
url: '${__url.path}/lib/${__value.text}',
},
])
)
.build();
// throw new Error("Cannot find any scene object that uses the key '" + key + "'"); | ||
return key; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happen if I try to get a scene object from this key? would I receive an undefined object or is it possible that any exception in other place will. be thrown?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yduartep not sure what you mean
This is really amazing. I think will make our dev live easier if we can define scenes applications like a normal React app using components, contexts and hooks. Every time I try to review or build a scene page for complicated views, become hard to understand and follow the whole tree of objects. Great work here...!! PLEASEEEEE continue working on this. It's really promising. |
packages/scenes-react/package.json
Outdated
@@ -0,0 +1,118 @@ | |||
{ | |||
"name": "@grafana/scenes-react", | |||
"version": "4.24.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this package versioning be fixed or independent of the scenes package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jackw should be same as scenes package, think lerna enforce this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lerna defaults to fixed but you can switch to independent strategy if you wanted to separate them. Maybe it doesn't make so much sense if they're tightly coupled though.
packages/scenes-react/package.json
Outdated
"peerDependencies": { | ||
"@grafana/data": "^10.4.1", | ||
"@grafana/runtime": "^10.4.1", | ||
"@grafana/scenes": "workspace:*", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be worth checking the version of lerna in this repo is compatible with the workspace:
protocol before merging. Looking at the canary release attached to this PR it's publishing without replacing it. https://unpkg.com/browse/@grafana/scenes-react@4.24.3--canary.734.9287358327.0/package.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jackw looks like lerna is not updating this, any ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I'd not realised this is peerDeps! There's an open issue about it in the lerna repo. I think you might need to match it to the version in the repo so lerna understands to update it when versioning the packages. "@grafana/scenes": "4.24.2"
or "@grafana/scenes": "^4.24.2"
might be better if consumers want to rely on patch / minor updates to scenes package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jackw what do you think is best here a peer dependency or a normal dependency? Switched to normal dependency and now lerna updates it correctly , but not sure what is best
Created some initial unit test, ready for review & merge. Here is a epic issue with follow-up tasks: #761 The biggest thing I do not like right now is the name conflicts. While the new package now allows for some components to use whatever name they want (and conflict with scene objects exported from main lib). A) Ignore for now and rename either scene object or react version later |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 For a anyone's trying to run the demo locally I had to add this to the demo.sh
scenesreact=$(pwd)/packages/scenes-react
yarn install
cd $scenesreact
yarn install
yarn build
Problem
It's been clear for a while that scenes have issues when going beyond simple dashboard-like declarative views.
These issues have troubled me greatly the 6+ months, I wish we had encountered these issues earlier in the process, we might have been able to rethink the architecture.
I did try a quick react context approach very early on but failed to find a way to nest contexts while controlling re-renders (and a few other issues, like how to cache state, and complex state logic among others). I wish I had pursued it more but was too focused on solving core/future persisted dynamic dashboard needs.
It's too late to start from scratch. So in this PR, I am trying my best to expose pure react components, contexts and hooks that re-use as much as possible of the existing scene fundamentals (variable system, query system, time range, VizPanel, URL sync).
Started this late yesterday, so still very early but looks more promising than I expected.
For a quick sense take a look at the usage examples
Components
Hooks
Also a new abstraction to define visualizations for the new react VizPanel component
Solved issues
Unsolves issues / todo
Thoughts
Contexts / state outside the routes / pages
By letting go of the "scene" concept as these serializable declarative app views (which scenes was primarily designed for) it's easier to have a state outside the pages/views (time range and variables that live above page routes or drilldowns). This would be possible in the plain scene model as well with some small changes. It's nice to be able to define the shared time range & variables that all child pages should have once. And they never need to be re-activated and get state sharing (form of caching) for free here.
Separation between panel and visualization
I really love the separation between the visualization definition (pluginId, pluginVersion, options, fieldConfig) and the panel options that RVizPanel give. Something I have been trying to do before with VizPanel but failed to come up with the right model/abstraction. I was trying to break it up into two components one dealing with PanelChrome and the other rendering the visualization plugin, but as the level that renders PanelChrome needs to handle info from both this approach failed in various ways. But by just defining one object that fully encapsulates the visualization we get the same effect (something we could do to VizPanel).
This separation enables you to define different visualization definitions and easily reuse them in your scene app. We can even ship some good defaults one with scenes lib.
Next steps
I think this looks too promising to no continue work on and get help from scene app devs who are interested. The dashboard / scene squad is a bit thinly stretched with dashboard migration & big customer POC work so any help is appreciated.
Things needed before merge / making it real
📦 Published PR as canary version:
4.24.5--canary.734.9315492006.0
✨ Test out this PR locally via:
npm install @grafana/scenes-react@4.24.5--canary.734.9315492006.0 npm install @grafana/scenes@4.24.5--canary.734.9315492006.0 # or yarn add @grafana/scenes-react@4.24.5--canary.734.9315492006.0 yarn add @grafana/scenes@4.24.5--canary.734.9315492006.0