ROKMETRO Updates #437
ROKMETRO Updates #437
Conversation
# Conflicts: # lib/service/config.dart
|
Hi @shurwit, It is better in the future to use the "classic" way and have standalone PRs for each separate feature that you want to integrate in app-flutter-plugin. This huge PR contains an ocean of changes containing too many different features. It is hard to observe and analyze and obviously it will get merged mostly as it is without a necessary analysis why we do something, are there alternative ways, and so on. I have some questions on your updates in the plugin: 1. What is the exact purpose of Service.initServiceFallback? When any service returns a fatal error on its initialization the only sensible action to tame (in my opinion) is to present the relevant UI to the user, explaining what happened and giving an option to retry again the service initialization. This is what Illinois app does. 2. What is the purpose to make NotificationsListener.onNotification asynchronous (i.e. to return a Future)? 3. Assets service 4. App Styles 5. Plugin assets (styles.json & timezone.tzf) I started integration of the changes coming from "rokmetro-updates" branch of app-flutter-plugin in Illinois app. I created "rokmetro-updates" branch in Illinois app repo for this. While some of the issues are clear and easy to fix there are other issues that I am not quite sure how to resolve. For example, I am not sure how to handle the changes in the Auth2 APIs. I am also making minor updates in "rokmetro-updates" branch of app-flutter-plugin but if you feel that something is wrong please let me know for this. Thank you! |
|
Hi @mihail-varbanov, thanks for looking at this and for providing your feedback!
Yes I definitely agree with this. This was the result of us working on independent projects with time pressure so neither you nor we had the bandwidth to review and merge these changes independently. With that being said I do think we can and should keep these more organized along the way to make the review process more manageable once we get to that point. I also hope to keep the plugin more aligned going forward in general to avoid these types of divergences whenever possible. Let me know if you have any other thoughts on how we should organize the process for continued alignment and contributions to the plugin going forward.
I am certainly not sure that the fallback initialization system we have here is an ideal solution but let me try to describe the situation that led to these changes. For the Vogue app they did not want to display an initialization error to the user if one occurred. Rather they just wanted to land on the login page and display an error if the user attempts to login. I am checking to see if the services initialized successfully or not at that point. If they did not I attempt to reinitialize before attempting to login and display an error to the user if the initialization fails again. All of this being said, the fallback initialization is intended to allow services to be initialized even if one of its dependencies could not be initialized. As an example it is possible to initialize the Styles service regardless of what happens to any other dependencies (ie. even if Configs fail to initialize). This is important because regardless of how we handle the error (displaying the initialization error and retry panel that you mentioned, showing the login page like we do for Vogue), we need to ensure that the correct styles are loaded in order to display the page appropriately. We have default style references included in the plugin that do need to be overridden by some form of Style service initialization for everything to look and work as expected. Hopefully this helps you understand the intention of this system. At the time I felt that this was the way to support the expected functionality with the least impact or risk to existing functionality, but I am very open to suggestions as I do feel that it is probably more complicated than it should be.
The intention here is to optimize performance around notifications. In general, the notification service is used to communicate between various services and UI components that do not have direct references to each other. Usually, this is used when one service performs an action that requires various other services to perform related actions or requires the UI to be updated (possibly in multiple places). I cannot think of many situations where the actions triggered by the notification need to be performed one by one or in any specific order. Even if they did need to be performed in a certain order, the notification service does not support this since the order is arbitrarily determined by when each notification listener was registered. This change was made because we noticed significant performance deterioration when many services or UI components are listening for the same notification. It does not make sense to me that we would wait for each one to complete before moving on to the next since as I described above there cannot really be any logical relation between each service/UI component Let me know if you think I am missing something, but I can say that this significantly improved the performance of the app where many listeners were added to the same notification since each listener was previously being called only after all actions were completed by the previous one.
We did intentionally bring this back at one point but I can no longer find the reason that we needed it. I have removed it as I agree that this should be retired as you intended.
I understand your concerns about the style references within the plugin, although I don't know that it is actually broken as you described. Since the style definitions in the plugin can and should be overridden by the app itself, these changes should be reflected within the UI components in the plugin. My introduction of the code generation tool (tools/gen_styles.dart) for the styles was also intended to help with this, as it will automatically merge in the styles.json definitions within the plugin into the app styles.json. This ensures that we do not forget about or overlook any of the definitions in the plugin when creating a new app. Are you seeing anything specific that is actually broken or not working as expected due to the style references? I personally feel that it is important that we have a strategy that allows us to define reusable UI components in the plugin that we can use across various apps if we want to. This dramatically reduces code duplication and speeds up development times. I know this is something that we discussed before and I don't believe we ever really settled the debate, but my feeling is that the goal of the plugin is to make reusable components like these available. Any input you have on how we can approach this in a way that is maintainable would definitely be appreciated, but my team has been using this system across a couple of apps now and so far it has worked pretty well for us.
These are both intended to be defaults that can be overridden by the app itself. All of the apps that we have built so far have used the same base definitions for both of these assets so I feel that not including them just creates more unnecessary work. For styles.json specifically, please see my note above about the code generation tool merging these styles above. This was added so that we can guarantee that the necessary style references are available so we can enable the creation of widgets that depend on these overridable styles.
Thanks for working on this! Yes, the Auth2 changes are not quite ready to be incorporated. As I mentioned above @roberlander2 is working on wrapping up a PR on the Core BB for @petyos to review. This is also a fairly substantial change so it will take some time to review. We will need to merge that PR and deploy to dev before we are able to test the Auth2 changes in the Illinois environment. Thanks again for all of your comments here! I know this is not an ideal workflow and I appreciate your willingness to work through this with us. My main goal with this PR is to help get us aligned so we can work more collaboratively going forward rather than further diverging on separate branches like this. Hopefully some of these changes will be helpful in the Illinois app too, but at the very least it will let us start fresh and begin contributing the plugin in a more sustainable way going forward. |
|
Hi @shurwit, Thank you for your feedback! 0. Use the "classic" way … 1. Service.initServiceFallback I do not like the initServiceFallback resolution because it appears to me like a patch. I think the source of the problem is somewhere else, it is in the not-precise service dependency definition. Styles, FlexUI or Localization do not really depend on Config service, as they state, because they can get initialized without the app config being available. They need Config service only for checking if there are some overrides from the Net, but this does not break their initialization. I think the right resolution is to tune and adjust the dependency of the services, their initialization process, and the use cases when we throw the fatal initialization errors. My belief is that once this get tuned and adjusted Vogue can present its login UI in case of fatal initialization error, and re-attempt Services initialization again on the next login attempt from the user. My suggestion is, if you do not mind, I to try to implement this for the Vogue app in a separate branches (in Vogue and the plugin), and if my resolution is fine for the Vogue app (you will judge that) to incorporate this update in the "rokmetro-updates" branch. Does this sound good to you? 2. NotificationsListener.onNotification 3. Assets service 4&5. App Styles / Plugin Assets While talking about "gen_styles.dart" tool, could you make a few minor improvements/fixes there:
Now I am switching back to Style definitions in the plugin. Generally yesterday I did not like the idea of having AppColors.fillColorPrimary definition in the plugin. The reason for this is that we cannot assume that all apps that host this plugin will have the same or similar styles definitions and structure like the Illinois's app. As far as I understand the only reason to have them in the plugin is to serve the UI components that reside in the plugin, e.g. RoundedButton, RibbonButton and so on. Is that so? I would like to take a stand on the subject about "having a strategy that allows us to define reusable UI components in the plugin". I have two notes:
Right now I'm just sharing with you my thoughts on this matter. I know you have a different opinion. I find this discussion useless at the moment because we are trying to predict what will happen in the future, i.e. what the other apps that will host this plugin would expect from it. In Bulgaria we have a saying "to put the cart before the horse". I mean we are trying to design a plugin that will serve some unknown client apps. The order must be the opposite: first see what clients exactly will use this plugin, find the intersection of their needs, and implement it in this shared and reusable component. So, what I am trying to say is that for now we can keep what we have in the plugin, and think on the possible solutions when other client app appear and the current model in the plugin does not apply well for them. 6. Integration of "rokmetro-updates" in Illinois app I agree with you that if we are going to share the same source codebase we should work more collaboratively together than we used to do. I see your efforts in this direction and I am grateful to you about this. |
|
Hi @mihail-varbanov, thanks again this is all very helpful!
I see exactly what you mean and agree that what you described here sounds like a better solution. I will leave it to you to take a pass at this dependency optimization as you suggested and I will test it out when you are done. Let me know if you have any questions about our use case along the way. Thanks for you help with this!
I'm glad this makes sense to you now! I'm not sure we even need the FutureOr vs. just switching to Future completely because I wasn't sure if there was a reason that it was built synchronously in the first place that I was overlooking and I did not want to force an update in the Illinois app across all notification listeners. That being said, if you do not see a reason that these should ever be synchronous I would suggest that we just go ahead and make all notification handler calls asynchronous. Let me know what you think about this. Thanks!
Thanks for the suggestions above! I'm also very glad that the style generation tool makes sense to you and that you also feel it alleviates some concerns about the use of styles in the plugin.
I do agree that there will be cases where the UI components need to be styled quite differently between applications. This will be the case whenever we are working with a customer that has strong pre-existing design and branding guidelines and wants us to build an experience that matches (ie. Vogue). On the other hand, we also have done and will continue to do projects where the customer defers to us on design considerations for the most part and just wants to provide us with some colors, fonts, and possibly icons (safer community, chronic illness app, memory app... etc.). In these cases, having standard reusable components makes everything much easier to implement, and as you can see the style system aligns well with our needs. All of that being said, I see your point about over-planning and trying to predict what will come (interestingly the exact phrase "putting the cart before the horse" is a saying in English as well). I think for the moment you are right we do not need to make any changes. I just wanted to come to some conclusion with you about the correct approach so that we are aligned as we go forward. Based on what you are saying it sounds like you are open to the idea of adding in more reusable UI components as we begin to see the need for them, which is what I was hoping to clarify. If this is the case, I would suggest that as we move forward and begin developing new applications we begin making PRs to add components that we feel are reusable (eg. if we want to use an existing component in the Illinois app or some other app that is not yet in the plugin, or if we create a component that we feel is fundamental and want to be able to easily access in future apps). We can then discuss on a case-by-case basis each one when you review the PR and decide if it makes sense or not. Does this sound ok to you?
Ok sounds good. I would suggest that our team can work on your Illinois app branch and help resolve the remaining build errors to prepare for the changes to the Core BB. We can test it out once @petyos has had a chance to review and merge @roberlander2's PR (which is available here for tracking purposes: rokwire/core-building-block#707).
Thanks for the acknowledgement and for all the feedback again! I feel like we are now fairly aligned on these issues and have a clear path forward! |
|
Hi @mihail-varbanov, just wanted to give you a quick update on your styles generation comments. I believe I have hopefully addressed both of your comments in the last couple of commits.
Please try it out now and see what you think. If there are any remaining concerns let me know. Thanks! |
|
Hi @shurwit, Thanks for your comments. Here are my replies and comments on them. 1. Service.initServiceFallback OK, thanks. I will start the work on this. I will ask you for some assistance/clarification if I need it in Vogue, and will let you know when I get ready. 2. NotificationsListener.onNotification
Nowhere in the source code does it wait for any notification to be processed, i.e. there is not a problem with the notification handlers being asynchronous (returning Future). However I think you will get exactly the same result as the current when they are actually FutureOr. The reason is that when Dart calls a function that is FutureOr it always does not "await" for its execution even if the function is actually synchronous and returning void (take a look here). When I see that some function returns Future I am expecting there to be performed some asynchronous processing, that is not exactly the case of NotificationListener's onNotification handlers. So to summarize, what I am trying to say is that I do not expect problems from changing onNotification to return Future instead of FutureOr, I just find it a little unnatural and maybe confusing for someone aside. However if you are strongly convinced that this will be for the better we can do it. 4&5. App Styles / Plugin Assets
I did not know that you are trying to merge plugin and app's styles.json. I do not think that this is a good idea and the right way to go, I mean to override the app's source code or resources (gen directory content is something else). If you take a look at Styles service you will see that it reads and merges the "default" (assets/styles.json) and "app" (app/assets/styles.json) style definitions, but this happens programmatically and does not modify any source data. This is the way that I designed to build apps that are very similar to Illinois, customizing just some colors, fonts, strings, etc. In my opinion this is the right way to go, if we need to merge plugin and app resources at all, please see bellow. In general, I think there should not be any resource definitions in the plugin. I would rather go towards removing them from the plugin and working on opening the plugin widgets to be easily customized (fonts, colors, etc.) by the host app, without making explicit references any entries from styles.json. Let me say it again, the Illinois app codebase is theoretically prepared (but never used) to support multiple build targets that can build apps that are similar to the Illinois app.
Thanks for the quick update. When I try to run the latest script version I get this error: This did not used to happen before.
I am sorry but this does not sound quite good to me. I think that adding a reusable UI in the plugin should happen only when more than a single host app needs it. Until this happens, the potentially reusable UI should reside in the plugin host app only. When another host app appears that needs the same UI, we can design the right model of the UI component that will go down to the plugin, analyzing the different requirements that come from the different apps. This is "to put the cart after the horse" for me. In general, I think that the plugin should contain the intersection of the reused UI across the host apps, not the union of everything used in these apps. I think that we should try to keep the plugin as small and simple as possible, not trying to grow it without а good and clear reason. 6. Integration of "rokmetro-updates" in Illinois app
Thanks, this is exactly what I was trying to suggest. |
|
Hi @mihail-varbanov, thanks!
Sounds good to me!
Thank you for bringing this to my attention. I spent some time looking into this and I actually think that I was mistaken about this entire thing and that the change we made here probably did not improve the situation at all. I had a fundamental misunderstanding of how the async functions work in Dart. Because Dart runs everything in a single isolate/event loop (https://dart.dev/language/concurrency) it seems like the code executed within an async function actually is performed synchronously unless it is waiting on some form of I/O stream. To test this out I wrote a minimal example that compares the execution and performance between the async and synchronous onNotification implementation: import 'dart:async';
void main() async {
List<NotificationsListener> listeners = [
FutureExample(), Example(),
];
for (NotificationsListener listener in listeners) {
print(listener);
DateTime time = DateTime.now();
for (int i = 0; i < 3; i++) {
listener.onNotification('call$i', i);
}
print(DateTime.now().difference(time));
}
}
class Example with NotificationsListener {
void onNotification(String name, dynamic param) {
int val = 0;
for (int i = 0; i < 200000; i++) {
for (int j = 0; j < i; j++) {
val = i*j;
}
}
print('sync: $name: $val');
}
}
class FutureExample with NotificationsListener {
Future<void> onNotification(String name, dynamic param) async {
int val = 0;
for (int i = 0; i < 200000; i++) {
for (int j = 0; j < i; j++) {
val = i*j;
}
}
print('async: $name: $val');
}
}
abstract mixin class NotificationsListener {
FutureOr<void> onNotification(String name, dynamic param);
}I tried running this and I found that even with the async function, the code is executed synchronously and sequentially just like the synchronous function. I actually found that it took longer to run the async functions, most likely due to additional overhead to set them up. See an example of the results below: I think I mistakenly thought that this change had an impact on the performance we saw in the app when in fact the improvement was due to some other changes that we were making along side this one. Let me know what you think, but based on the results here I actually feel that it is best to revert this change completely to the current state on develop at this point. I am glad that I learned more about how the async functions operate in Dart from this, but I apologize for the unnecessary work looking into it.
I think there are tradeoffs here and I see both sides of the argument. I think it will be a little tricky to communicate like this to address this. I spoke to @pmarkhennessy earlier today about this and I believe he would like to discuss with you on a call to make sure we all understand each other's perspective well. As for the error you mentioned, thanks for catching that. I pushed some updates that hopefully resolve a couple of issues including that one.
I think if I am understanding you correctly you are taking issue with my statement that I would like to add components to the plugin "if we create a component that we feel is fundamental and want to be able to easily access in future apps" but not the part about adding components when "we want to use an existing component in the Illinois app or some other app that is not yet in the plugin." If this is indeed what you mean then I am happy to move ahead with this approach as I agree that we would not want to clutter the plugin with unnecessary UI and it should not be too hard to move the components in the future if they ever are needed in multiple apps. If you feel I misunderstood your point here please let me know. Thanks!
Great, we'll get started on this soon. |
|
Hi @shurwit, thank you! 1. Service.initServiceFallback I created two PR with the suggested workaround: #439 and https://github.com/rokmetro/vogue-app/pull/713 2. NotificationsListener.onNotification I was wrong too. I knew that everything is executed on the main thread/isolate. However I was expecting that when using FutureOr as a return type the onNotification handlers will get invoked after NotificationService.notify call finishes, and this helps you somehow to improve the performance. Today I played with this and found that this is true only if we wait for async calls in onNotification handlers (something that we normally do not do), and also that there is absolutely no difference if onNotification return type is void or FutureOr. I think it is safe and better to return it to void as it was initially. 4&5. App Styles / Plugin Assets
I am looking forward to this conversation.
This is exactly what I had in mind. It is great that we are on the same page. |
Tuned services initialization, get rid of initServiceFallback [#408].
# Conflicts: # lib/service/firebase_messaging.dart
Description
This PR contains changes, fixes, and improvements from the ROKMETRO team. These changes were introduced and used by a variety of other applications. The goal of this PR is to align the Illinois app and the rest of our apps on a single branch of the plugin.
We understand that there are a lot of changes here and some of them are quite significant, so all feedback is very welcome. Please let us know if you have any suggestions, concerns or thoughts about the changes we made here.
Thank you!
Depends on: rokwire/core-building-block#674
Auth2are mostly related to this issue