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
DynamicApp: Demo of a SceneApp with loading phase, a first setup phase and settings page that rebuilds pages #466
Conversation
isConfigured?: boolean; | ||
} | ||
|
||
export function getDynamicApp(model: SceneObject): DynamicApp { |
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.
Interesting trick!
I'm facing the same issue in a refactoring I'm working on, but solved it by dispatching an event to bubble up from the "landing page" scene and listening to it in the SceneApp
.
It feels though like we're at a point where we'd benefit from a state management solution that's loosely coupled with the scene object hierarchy, to avoid a flimsy tangle of interconnections. "context for scenes" or "redux for scenes" or some such. Already seen cases of variables being (mis)used for 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.
Not only some state management, but some ability to store plugin settings :D
export function loadSettings(): Promise<AppSettings> { | ||
let settings: AppSettings = {}; | ||
|
||
const settingsJson = localStorage.getItem('dynamic_app'); |
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.
My first question was where the settings would be stored. This could be a good time to light a fire under some of the efforts to provide plugins with some persistent storage, then this would be extra helpful by providing an API to read/write.
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.
@jewbetcha this use of localStorage was just a "simulated" real store (a real plugin would use an http API to fetch / save settings).
Still waiting on app platform work to support new storage & apis for app plugins
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.
Nice, yeah I'm all for this!
export class DynamicApp extends SceneObjectBase<DynamicAppState> { | ||
constructor(state: DynamicAppState) { | ||
super(state); | ||
this.addActivationHandler(() => { | ||
this._activationHandler(); | ||
}); | ||
} | ||
|
||
private async _activationHandler() { | ||
if (!this.state.settings) { | ||
// Set loading page | ||
this._updatePages([getLoadingPage()]); | ||
} | ||
|
||
const settings = await loadSettings(); | ||
this.setState({ settings }); | ||
|
||
if (settings.isConfigured) { | ||
this._updatePages(buildAppPages(settings)); | ||
} else { | ||
this._updatePages([getFirstSetupPage()]); | ||
} | ||
} | ||
|
||
private _updatePages(pages: SceneAppPage[]) { | ||
const app = sceneGraph.getAncestor(this, SceneApp); | ||
app.setState({ pages }); | ||
} | ||
|
||
public onChangeSettings = (settings: Partial<AppSettings>) => { | ||
this.setState({ settings: { ...this.state.settings, ...settings } }); | ||
saveSettings(this.state.settings!); | ||
|
||
if (this.state.settings?.isConfigured) { | ||
this._updatePages(buildAppPages(this.state.settings!)); | ||
} | ||
}; | ||
|
||
public onCompleteSetup = () => { | ||
this.onChangeSettings({ isConfigured: true }); | ||
locationService.push(prefixRoute('dynamic-app')); | ||
}; | ||
} |
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.
This is very interesting. Wonder how much we should consider this an API vs a pattern? I mean - this implementation is quite loosely coupled with functions (i.e. getLoadingPage
or getFirstSetupPage
) making it easy to actually extend/modify in any way one would need.
Thinking of a different scenario, where the first setup is not necessary, as the configuration is provided via default settings. I assume then it's the API that would be just return settings.isConfigured = true
, which defines an API of the persistent app settings to some extend.
My point is to learn how we want to solve this problem.
@domasx2 / @jewbetcha - would a DynamicApp API be something that you would fancy in your apps, or rather a pattern/guideline for solving a certain problem?
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 thought about it, but couldn't figure out an abstraction that felt useful
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.
@domasx2 @dprokop I had the same idea, that maybe this "loading/user setup/running" phase could be built into SceneApp some way. I think that could be interesting if we can find the abstraction / base class or what ever to make that possible.
Still struggling finding the right future for SceneApp, discussed some ideas here #470
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 DynamicApp API sounds really nice, especially since I know a lot of plugin teams (including us) have to do things like make a query for some metrics to see if setup has been complete.
Something like this could get rid of those types of workarounds 🎉
I have noticed that it's tricky to use SceneApp and SceneAppPages when you need a loading and setup phase where you either need to load settings and/or have the user make some initial configuration before all the pages can be built.
This is an exploration in how we can solve this.
Instead of extending SceneApp I am using a behavior attached to the SceneApp. This is because extending SceneObjects is problematic due to Typescript and generics co-variance and problematic from a backward compatible way (makes it hard to make changes to the library).
2023-11-16.13-38-16.mp4
Futher SceneApp exploration
I think SceneApp could be improved. Maybe support a cleaner way to extend than a behavior like in this PR and a easier way to access it (without having to call getParent on SceneAppPage)