Repository navigation
App level UINode build is called many times #20
Description
Testing the sky framework now with the stocks example. I added a print() statement to build() fn in stocks_app and discovered that it is printed close to 30 times in logcat:
05-18 12:51:30.268: I/chromium(8515): [INFO:builtin_natives.cc(144)] CONSOLE: Building the app node
05-18 12:51:30.330: I/chromium(8515): [INFO:builtin_natives.cc(144)] CONSOLE: Building the app node
05-18 12:51:30.410: I/chromium(8515): [INFO:builtin_natives.cc(144)] CONSOLE: Building the app node
05-18 12:51:30.481: I/chromium(8515): [INFO:builtin_natives.cc(144)] CONSOLE: Building the app node
05-18 12:51:30.563: I/chromium(8515): [INFO:builtin_natives.cc(144)] CONSOLE: Building the app node
05-18 12:51:30.619: I/chromium(8515): [INFO:builtin_natives.cc(144)] CONSOLE: Building the app node
...
Just filing an FYI bug. I expected this to show up only once:
UINode build() {
print("Building the app node");
List<UINode> overlays = [];
addMenuToOverlays(overlays);
_scaffold = new Scaffold(
header: _isSearching ? buildSearchBar() : buildActionBar(),
content: new Stocklist(stocks: _stocks, query: _searchQuery, app: this),
fab: new FloatingActionButton(
content: new Icon(type: 'content/add_white', size: 24), level: 3),
drawer: buildDrawer(),
overlays: overlays
);
return _scaffold;
}
Activity
mehmetf commentedon May 18, 2015
It seems I don't fully understand the lifecycle of the UI components in the stocks demo. Is the app node build called every time the stocks data list changes? This would be consistent with the number of json data files. If so, it would make sense to rebuild the stock list only (or even reusing the existing stocklist component).
This is an important sample app since it showcases the how model changes should affect the UI. It would be great if this example was sanitized a bit.
Disclaimer: I have no experience in writing react apps which is what Sky seems to model.
eseidelGoogle commentedon May 18, 2015
Build should only be called once to generate the new tree after setState() has been called on the app. @Hixie
Hixie commentedon May 19, 2015
It should be called once after each call to setState(). setState() is called once for each data file.
As far as I can tell this is consistent with the model here. When the application changes state, you rebuild the application.
We could make the stock data be state of the list instead of state of the app, if we wanted to reduce the load here. We should look at a profile first to see if this is particularly expensive.
mehmetf commentedon May 19, 2015
Thanks @Hixie . You confirmed my observation.
I don't think what you are doing is particularly expensive for this sample app. However, it would be great if the sample was written with good design practices in mind. Making the stock data to be the state of the stock list is what I would have done except I don't quite know how to do it and it would be nice if the sample app showed that to me.
Hixie commentedon May 19, 2015
The data's lifetime is greater than the list's lifetime, in principle (e.g. consider if we had a way to see specific stocks' data, hiding the list -- the list would be unmounted, but the data would need to survive so that when we went back to the list, we didn't have to hit the network). So I think the right practice may in fact be to have it be in the app.
@abarth may have opinions too.
abarth commentedon May 19, 2015
We could move the ownership of the data lower down in the component hierarchy in the current app, but at some point in the future we might want to add UI in other parts of the app (e.g., the drawer) that depends on the stock data. Rebuilding all the UINodes from scratch shouldn't be that expensive an operation, however, as long as you end up with the same physical UI.
mehmetf commentedon May 20, 2015
Very good discussion guys, thanks!
@abarth I agree with you. If stocks data was somehow tied to the app, it should be at app level. A better example might be user data for logged in user. When that changes, typically the whole app should be refreshed.
In the context of the sample stocks app, you are reinitializing a scaffold with the same menu options etc along with a different stocklist instance for content. It did not feel natural. I think this could be a good case for showing how we could reuse scaffold and even the stocklist instance and simply rebuild the stocklist node. Again, note that I am not talking about performance but good design. For instance, the current design makes it difficult to switch scaffold's content node to something else since every time app build gets called the current content node is overwritten by stock list. I will send you a PR to show you what I mean.
mehmetf commentedon May 20, 2015
Take a look at this:
https://github.com/mehmetf/sky_sdk/commit/3388139658f613c70680539c0a0419da9953482f
I had to create an absurd variable called _contentFinal to decide whether we render stocklist or not. This is because stock list app is not really designed to have multiple pages. It would be great if you could comment/work with me on this to convert it to something where we can wire various pages to menu items etc.
Hixie commentedon May 20, 2015
The components you build in build() shouldn't be stored outside of build(). The point of build() is to generate the app's current state at that point. It is itself stateless.
Every time the app's build() is called, if you're displaying the stock list, you create a new StockList. If you're displaying something else, you build that instead. The stock row should definitely not be manipulating state in the app; it can have an event listener that the app listens to and then the app itself can change its state to be something, but not the other way around. You definitely shouldn't need to pass the App object around.
mehmetf commentedon May 20, 2015
Sounds good. The advice about build() is the kind of thing I am looking for these samples to teach me. Guidelines about what to do/what not to do in a Sky app.
As for event listener; I added one, PTAL: https://github.com/mehmetf/sky_sdk/commit/1d69b20728c105c6ea8a2c17353c34ee195951b0.
Presumably, this would be solved by the router component of the framework. Are you folks talking to Angular team? They would be interested in getting Angular 2 in there in which case you will get a router for free.
Switch to using the Fuchsia buildtools. (flutter#20)
github-actions commentedon Sep 7, 2021
This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of
flutter doctor -vand a minimal reproduction of the issue.