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

Persistence Manager #733

Draft
wants to merge 11 commits into
base: develop
Choose a base branch
from
Draft

Persistence Manager #733

wants to merge 11 commits into from

Conversation

Ryanseanlee
Copy link
Contributor

No description provided.


export const portfolioPanel = hoistCmp.factory({
model: creates(PortfolioPanelModel),

render() {
return panel({
tbar: tbar(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the tbar component is so minimal and doesn't react to any state changes, I would just specify the tbar items inline (i.e. tbar: [persistenceManager()])


export const portfolioPanel = hoistCmp.factory({
model: creates(PortfolioPanelModel),

render() {
return panel({
tbar: tbar(),
mask: 'onLoad',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably want to mask while the PersistenceManagerModel is loading views for the first time. Otherwise, this will render unmasked with whatever the default view is (in code) and then jump to the persisted view once it loads


this.persistenceManagerModel = new PersistenceManagerModel({
type: 'portfoioExample',
noun: 'portfoio',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sp: portfolio :)

.linkTo(this.initTask);
}

private async _initAsync() {
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 our convention here would be to call this doInitAsync rather than the underscore (that tends to be used more for private properties than methods). But also curious why split this out at all. Seems like it could all just go in initAsync

});

await waitFor(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple thoughts --

  1. lastLoadCompleted is observable, so you can use mobx wait here instead of waitFor. The difference is that waitFor polls on your specified interval, whereas wait should run as soon as the observable changes such that the condition in the callback is met.
  2. Like we discussed lastLoadCompleted runs whether doLoadAsync resolves or rejects. This is a little risky, but our whole story around a failed load is at the moment... let's talk about this some more, or maybe just flag it as something we should revisit

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, this is an annoying amount of boilerplate you had to write! I wonder if there'd be a way to generalize this and build a higher order component / model to avoid some of this. Just brainstorming - what if the PersistenceManager component was a container that accepted a single child component that it would not render until it was done initializing. Big issue here is that ideally the child would create its model, so that it would not be constructed until the manager finished initializing. BUT we would need a reference to that model in order to wire it. Wonder if we could use modelRef to do that... but we wouldn't want whatever we do to be even more complicated / harder to reason about than what you have here...

Sorry for the flight of ideas - will think more on this, and maybe we can brainstorm together.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants