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

PlainReact: Expose scene features through contexts and hooks and normal react components #734

Closed torkelo closed 3 months ago

torkelo commented 4 months ago

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
:sparkles: Test out this PR locally via: ```bash 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 ```
jewbetcha commented 4 months ago

This looks awesome! Great stuff @torkelo

Elfo404 commented 4 months ago

This looks very promising, will take a look asap!

oscarkilhed commented 4 months ago

Started implementing some demo stuff to try this out, ran into a build issue and made this pr to this branch: https://github.com/grafana/scenes/pull/741

ivanortegaalba commented 4 months ago

@torkelo "This looks too promising to stop working on it". I completely agree.

This is kind of the expected API a dev will expect to create UI and reuse dashboard components. I want to keep playing with it, but my first feeling is "this should be the default" to use Scenes. We don't even need to mention scenes anywhere, instead, we should only use dashboard domain names and components. Scenes will be the engine under the hood, but IMO, the way to develop Dashboards on plugins should be something like what we see in this PR.

About how to expose them, I tried to answer in the previous paragraph. The default components should be the react components, and the ones namespace should be the Scenes ones. Is not any of the proposals you mentioned before, but once this approach is validated, I'd advocate for a breaking change to make the React components the default ones.

matyax commented 4 months ago

+1 @ivanortegaalba

we should only use dashboard domain names and components

I'm not sure if I'm misreading your comment there, but I wanted to add that this will also be used to build applications (like Explore Logs), so I would advocate for not using/exposing dashboard-specific terms. For example, just today I had to work with a behavior within Explore Logs using an enum called DashboardCursorSync https://github.com/grafana/explore-logs/blob/main/src/Components/ServiceScene/ServiceScene.tsx#L440

yduartep commented 4 months ago

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.

torkelo commented 4 months ago

Created some initial unit test, ready for review & merge.

Here is a epic issue with follow-up tasks: https://github.com/grafana/scenes/issues/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). I don’t really like having two things with the same name (bad for code navigation, auto import, etc). Especially for VizPanel, having a react component named VizPanel and scene object named the feels super bad. Options

A) Ignore for now and rename either scene object or react version later B) Rename new react versions VizPanel => VizPanelView or PanelViz or Panel CustomVariable => DefineCustomVariable (since these components don’t render anything, they just add a scene object to closest context)

dprokop commented 3 months ago

Quick find: after navigating away from Dynamic queries demo, there's a runtime error happening:

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)
torkelo commented 3 months ago

@dprokop yea, noticed that yesterday as well. bug introduced in my simplification commit, was a simple bug/mistake in addVariable clean up function

torkelo commented 3 months ago

@mdvictor demo.sh ? not sure what that does.

the simplest way is just yarn dev this will build the two libs and the demo app (in parallel watchers).

then in your local grafana dev folder conf/custom.ini

[plugin.grafana-scenes-app]
path = /Users/torkelodegaard/dev/scenes/packages/scenes-app

update to what ever path you have to the scenes repo

mdvictor commented 3 months ago

@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!

grafanabot commented 3 months ago

:rocket: PR was released in v4.26.1 :rocket:

matyax commented 3 months ago

What would be the best way to keep track on the development of this Scenes flavor to consider porting apps to it?

torkelo commented 3 months ago

@matyax keep an eye on https://github.com/grafana/scenes/issues/761 and for this to be usable any time soon I hope many scene app devs will help with issues in the epic :)