Skip to content
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

Open
wants to merge 56 commits into
base: main
Choose a base branch
from

Conversation

torkelo
Copy link
Member

@torkelo torkelo commented May 11, 2024

Problem

It's been clear for a while that scenes have issues when going beyond simple dashboard-like declarative views.

  • Steep learning curve
  • Hard to mix with familiar React patterns & state management & hooks
  • The clonable & serializable state model that can be walked is very rarely needed in scene apps (beyond internal scene mechanics)
  • Requiring all data, variables, queries and visualizations be added to a state tree makes rendering transient views unnecessarily cumbersome

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

  •  SceneContextProvider that represents a scene, accepts defining an optional time range scope. This also acts as a scope for SceneVariableSet. Can be nested
  • CustomVariable. Adds a CustomVariable to closest scene context.
  • VariableSelect. Renders a variable value selector (no matter what level the variable is defined on)
  • TimeRangePicker. Render the time range picker using closest scene time range
  • VizPanel renders panels & visualizations with provided data provider. Has a very nice separation between panel props (title, etc) and visualization definition
  • RefreshPicker renders the SceneRefreshPicker.
  • BreadcrumbContext & BreadcrumbContextProvider & Breadcrumb (Helps with maintaining state in breadcrumb URLs)

Hooks

  • useTimeRange
  • useVariables
  • useVariableValues returns the values of a specific multi-value variable (and updates/re-renders the calling component when values change)
  • useQueryRunner. Adds a SceneQueryRunner to the closest scene context. Returns the query runner / handles updates to queries.
  • useUpdateWhenSceneChanges (Re-renders components when specified variables or time range changes)
  • useVariableInterpolator. Hook that returns an interpolation function, also re-renders the calling component when variables or time range change.

Also a new abstraction to define visualizations for the new react VizPanel component

  • VizConfigBuilders. A new form of builder that only defines a visualization (pluginId, options, fieldConfig)

Solved issues

  • Pattern for dynamically adding/removing scene objects to a scene (context) as they are rendered
  • Dynamically add variables via rendering a react component
  • URL sync works for dynamically added variables & time ranges (needs more work to handle key conflicts)
  • Figure out how we can use these new hooks & components from inside a plain (ie current) scene. To make it possible to mix the approaches (to aid migration & maybe other reasons). EmbeddedScene can now optionally wrap the scene in a SceneContext.
  • Support query controller

Unsolves issues / todo

  • No caching of RVizPanel state. For example if you toggle a legend in a graph and then go to another page and come back.
  • No caching of data (returning to view re-queries data).
  • URL sync for dynamically added objects whose key conflicts with existing objects. For example 2 time ranges.
  • URL sync when changing path (currently UrlSyncManager skips this expecting a full sync will happened when path changes and a new scene is initialized)
  • Lots of missing props on VizPanel
  • Some missing props on useQueryRunner
  • No layouts that work with this model (SceneFlexLayout & CSSGridLayout)
  • Transformations (should be easy)
  • Data layers (annotations) (should be relatively easy)
  • Minor issue with RefreshPicker flickering on the first mount (need to refactor this to be pure react component rather than proxy scene object). very low prio issue, but annoying.

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

  • Where do these new components live (Moved to new scenes-react package)
  • Agree on naming of components.
  • Reverted URL sync changes, leaving them for follow-issue
  • Some initial tests of the context & react components & hooks (could also be done in a follow-up issue if carefully defined and followed-up)
📦 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

Comment on lines 4 to 16
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();
Copy link
Member Author

@torkelo torkelo May 11, 2024

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

Copy link
Member Author

@torkelo torkelo May 13, 2024

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();

Comment on lines 14 to 15
// throw new Error("Cannot find any scene object that uses the key '" + key + "'");
return key;

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?

Copy link
Member Author

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

@yduartep
Copy link

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 torkelo added the release Create a release when this pr is merged label May 29, 2024
@@ -0,0 +1,118 @@
{
"name": "@grafana/scenes-react",
"version": "4.24.0",
Copy link
Contributor

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?

Copy link
Member Author

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

Copy link
Contributor

@jackw jackw May 29, 2024

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.

"peerDependencies": {
"@grafana/data": "^10.4.1",
"@grafana/runtime": "^10.4.1",
"@grafana/scenes": "workspace:*",
Copy link
Contributor

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

Copy link
Member Author

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?

Copy link
Contributor

@jackw jackw May 29, 2024

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?

Copy link
Member Author

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

@torkelo
Copy link
Member Author

torkelo commented May 31, 2024

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).
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)

@torkelo torkelo requested a review from dprokop May 31, 2024 08:37
@L2D2Grafana L2D2Grafana self-requested a review May 31, 2024 20:14
Copy link

@L2D2Grafana L2D2Grafana left a 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release Create a release when this pr is merged
Projects
Status: 🔍 In review
Development

Successfully merging this pull request may close these issues.

None yet