PlainReact: Expose scene features through contexts and hooks and normal react components #734
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.
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.
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();
|
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 |
|
Quick find: after navigating away from Uncaught TypeError: Cannot read properties of undefined (reading 'setState')
at eval (SceneContextObject.js:68:11)
at safelyCallDestroy (react-dom.development.js:22932:1)
at commitHookEffectListUnmount (react-dom.development.js:23100:1)
at commitPassiveUnmountInsideDeletedTreeOnFiber (react-dom.development.js:25098:1)
at commitPassiveUnmountEffectsInsideOfDeletedTree_begin (react-dom.development.js:25048:1)
at commitPassiveUnmountEffects_begin (react-dom.development.js:24956:1)
at commitPassiveUnmountEffects (react-dom.development.js:24941:1)
at flushPassiveEffectsImpl (react-dom.development.js:27038:1)
at flushPassiveEffects (react-dom.development.js:26984:1)
at commitRootImpl (react-dom.development.js:26935:1) |
|
@dprokop yea, noticed that yesterday as well. bug introduced in my simplification commit, was a simple bug/mistake in addVariable clean up function |
| class CustomSceneObject extends SceneObjectBase<SceneObjectState> { | ||
| static Component = ({ model }: SceneComponentProps<CustomSceneObject>) => { | ||
| const [timeRange, _] = useTimeRange(); | ||
|
|
||
| return <div>Time hook: {timeRange.from.toString()}</div>; | ||
| }; | ||
| } |
There was a problem hiding this comment.
This is a great, I think the hooks could be easily used in SceneReactObject as well to allow some apps to start using the scenes-react APIs (without suddenly rewriting everything to scenes-react).
Yesterday I talked with @juanicabanas about a certain case where he wants to access DashboardScene down deep in the hierarchy that's rendering a dummy react component which needs an access to the root DashboardScene. Today, this requires props drilling or manually setting up a react context, that provides the required scene object . If the DashboardScene for example was able to be set up itself as a context provider, then this interoperability would be very easy to achieve. But this will always yield a problem of a making it type safe- how would the consumer down the hierarchy know what is the type of the resolved context object if the hook is generic :(
There was a problem hiding this comment.
@dprokop Sadly not every SceneObject can act as a context right now, but DashboardScene can easily wrap its content in a SceneContextProvider (with itself as the context's parent). this would enable hook & react components usage. But this context would only get the global time range and variables, so it would not work for local contexts, say on panel or row level (if we ever add time range to them) this would need nested SceneContextProviders.
Maybe some way to simplify this and make any scene object act as a context value but would require some new universal props on SceneObjectState and functions on SceneObject interface. Think we can revisit this after we see how the interoperability turns out
| <Switch> | ||
| <Route path={`${urlBase}/drilldown`} component={DrilldownHome} exact /> | ||
| <Route path={`${urlBase}/drilldown/lib/:lib`} component={DrilldownLibraryPage} /> | ||
| </Switch> |
There was a problem hiding this comment.
This is my fav demo, the simplicity if building drill down pages is just 💯 compared to SceneAppPage.
|
@mdvictor demo.sh ? not sure what that does. the simplest way is just then in your local grafana dev folder conf/custom.ini [plugin.grafana-scenes-app]
path = /Users/torkelodegaard/dev/scenes/packages/scenes-appupdate to what ever path you have to the scenes repo |
|
@torkelo the script builds everything and starts grafana in a docker container. But yeah, the way you suggested is similar to how I ended up using it. Thanks! |
| export interface VizPanelProps { | ||
| title: string; | ||
| dataProvider?: SceneDataProvider; | ||
| viz: VizConfig; |
There was a problem hiding this comment.
a nit, but i would call this config, the viz property name says very little about what's expected here (and what it actually gets)
There was a problem hiding this comment.
@dprokop maybe vizConfig to be extra clear?
There was a problem hiding this comment.
yeah, tho i think config should just work fine, the context is VizPanel, don't think we need to be over-expressive here.
There was a problem hiding this comment.
@dprokop not sure I like it after testing, we would have config.options, and config.fieldConfig etc, compared to viz.options , viz.fieldConfig which I think read much better
alternative maybe is visualization , but since we have VizConfigBuilders and type of viz is VizConfig, think `viz´ is ok
| @@ -65,6 +65,8 @@ function EmbeddedSceneRenderer({ model }: SceneComponentProps<EmbeddedScene>) { | |||
| </div> | |||
| </div> | |||
| ); | |||
|
|
|||
| return inner; | |||
There was a problem hiding this comment.
@dprokop yea, remnant from where I conditionally wrapped the inner content in a SceneContext, reverted this change now so this file should be unchanged
| this._sceneRoot = root; | ||
| this._lastPath = locationService.getLocation().pathname; | ||
| this._lastPath = location.pathname; | ||
| this._urlParams = new URLSearchParams(location.search); |
There was a problem hiding this comment.
I assume this is to capture the url params when the scene rendered, so that later on, when syncing, we can get the actual params, not the ones that got updated on navigation?
There was a problem hiding this comment.
@dprokop will actually revert this change as the reason I added it is no longer part of this PR (it was to access latest urlParams in a function syncNewObj, but this function is removed in this PR, doing all the URL sync changes in a separate PR
|
🚀 PR was released in |
|
What would be the best way to keep track on the development of this Scenes flavor to consider porting apps to it? |
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
For an explanation of the approach, there is a design doc
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.26.1--canary.734.9385486259.0✨ Test out this PR locally via:
npm install @grafana/scenes-react@4.26.1--canary.734.9385486259.0 npm install @grafana/scenes@4.26.1--canary.734.9385486259.0 # or yarn add @grafana/scenes-react@4.26.1--canary.734.9385486259.0 yarn add @grafana/scenes@4.26.1--canary.734.9385486259.0