UrlSync: Support browser history steps, remove singleton

[torkelo]

• Adds new function to SceneObjectUrlSyncHandler named shouldCreateHistoryStep that enable scene apps to control when new history steps should be created due to state changes
• Adds new feature to UrlSyncManager to sync initial scene state to URL, this is important as we need this initial URL state to go back to after a say a time range state
• remove getUrlSyncManager and remove the singleton nature of UrlSyncManager (needed for other reasons Make UrlSyncManager non singleton #841)

To not make this a big new behavior I am adding new options to control it

• updateUrlOnInit
• createBrowserHistorySteps (only SceneTimeRange is taking advantage of this)

can be set on SceneApp or UrlSyncContextProvider / useUrlSync

Question is should they be inverted and so the default state is enabled (ie updateUrlOnInit => disableUpdateUrlonInit, createBrowserHistorySteps => disableBrowserHistorySteps), so it's more opt-out than opt-in

Release notes

getUrlSyncManager is no longer exported as UrlSyncManager is now no longer global singleton but local to the UrlSyncContextProvider.
If you called getUrlSyncManager().getUrlState that util function is available via the exported object sceneUtils.

📦 Published PR as canary version: 5.13.0--canary.878.10702174754.0

✨ Test out this PR locally via:

npm install @grafana/[email protected]
npm install @grafana/[email protected]
# or 
yarn add @grafana/[email protected]
yarn add @grafana/[email protected]

[adrapereira]

Awesome, thanks! Will review later today but leaving my two cents on this:

Question is should they be inverted and so the default state is enabled (ie updateUrlOnInit => disableUpdateUrlonInit, createBrowserHistorySteps => disableBrowserHistorySteps), so it's more opt-out than opt-in

I would keep it as is since making it opt-out might cause unwanted behaviour on existing apps. As an app developer I would rather go and enable this as needed than update the scenes library and find out a few days later that something isn't working as expected and having to release a patch to disable the new behaviour.

[ifrost]

If I read this correctly it will provide newUrlState to shouldCreateHisotryStep including all keys. That wouldn't allow deciding to create a history step only for a given key, right? For example I may have two key/props synced to the URL but I want to create a history step only when one of them changes.

What about changes that happen in a batch or are chained? Coming back to the example you brought in #760

Let's say you have 3 variables (datacenter => cluster => job) where cluster depends on datacenter, job depends on cluster etc.
And you change datacenter in the UI, which automatically causes cluster to change, then job changes automatically as they are chained. You hit browser back and you get back to the previous value for job (which does not exist for the selected data center and cluster).

What if we needed to create a history step but only upon a user action, e.g.:

1. user selects a datacenter -> new history is created
2. cluster is changed automatically -> no new history is created, just updates the recent one
3. job is changed automatically -> no new history is created, just updates the recent one

Later:

4. user changes cluster manually -> new history is created
5. job is changed automatically -> no new history is created, just updates the recent one

Could (2) and (4) could be differentianed in URLSyncManager?

Maybe such case doesn't need to be supported and scenes would provide only simply API to get it out of the box and for anything more complex developers need to implement a bit of history handling.

One alternative option I was thinking was to have a flag passed to scene.setState(..., userAction=true) or variablechangeValueTo(..., userAction=true) or have another method but it feels cumbersome and clunky. Or have a trusted DOM event passed as an argument, but still feels a bit too much.

Another options I was thinking was to have something like:

getURLSyncManager().act(() => {
  variable.changeValueTo(...)
});

It would create a history step before calling a callback or temporarily set some global flag indicating that the next update to URL should be also pushed to history. Though it feels wrong too.

[torkelo]

What about changes that happen in a batch or are chained? Coming back to the example you brought in #760

It's tricky, the interesting thing to consider is when you do a new history your basically creating a step you can go back to. So time ranges changes (when initiated by the user at least) should be good "states".

What if we needed to create a history step but only upon a user action, e.g.:

You should be able to control this by control when shouldCreateHistoryStep return true (say right after a user action, but if it's a programmatic action you maybe return false).

user selects a datacenter -> new history is created
cluster is changed automatically -> no new history is created, just updates the recent one
job is changed automatically -> no new history is created, just updates the recent one

I have not implemented this for variables yet but my current thinking is to only create history items for calls to changeValueTo (as this is what the UI select calls), but when a variable changes value due to a parent / options change the this does not go through changeValueTo. So to handle this there would be some temporary state (next call to to shouldCreateHistoryStep should return true inside MultiValueVariable)

[ifrost]

You should be able to control this by control when shouldCreateHistoryStep return true (say right after a user action, but if it's a programmatic action you maybe return false).

The only argument that is passed to shouldCreateHistoryStep is the result of getUrlState(). How could we differentiate user vs programmatic actions based on this? 🤔

but when a variable changes value due to a parent / options change the this does not go through changeValueTo.

Sounds interesting! Do you think we could have something similar not only for variables but also for setting state of a scene object in general?

[torkelo]

@ifrost hm.. looks like GroupByVariable uses changeValueTo in updateFromUrl

so maybe we need a new parameter to changeValueTo(value, text, isUserAction = false)

[torkelo]

did a quick POC here, #882 works really well

[ifrost]

I tried to play with it a bit and works really nice! 🙌

What about the case where a non-variable state is synced with the URL and should be pushed to history. Would consumers just would write customer wrapping like below?

  public setFoo(value: number) {
    this._urlSync.performBrowserHistoryAction(() => {
      this.setState({ foo: value })
    })
  }

[torkelo]

@ifrost yes, something like that

[ivanortegaalba]

Should we allow to config updateUrlOnInit={true} createBrowserHistorySteps={true} here? Or the plugin dev should use a different UrlSyncContext provided to override those?

[torkelo]

@ivanortegaalba well scenes react is kind still not used for real anywhere, so figured we can leave these enabled by default until for now

[ivanortegaalba]

Took me a while to familiarize myself with all URL state management. The changes look good and the API is familiar and easy to use. I would add some documentation and examples to the scenes doc, but we can leave it for a follow-up PR.

✅ The time range changes add a new history entry
✅ The initial time range doesn't add an extra history entry
✅ Resolving variables A -> B, when B is resolved, A state update doesn't add extra history entry
✅ The default dashboard time range is not updated when going back in the browser.

All scenarios were tested in Dashboards, enabling updateUrlOnInit and createBrowserHistorySteps.

[torkelo]

@ivanortegaalba pushed some basic docs, we had zero docs on url syncing :)

[grafanabot]

🚀 PR was released in v5.13.0 🚀