Modular UI proof of concept #318
Conversation
# Conflicts: # dist/alpheios-components.js # dist/alpheios-components.js.map # dist/alpheios-components.min.js # dist/style/style.css.map # src/lib/l10n/message-bundle.js
…re added to UI controller's public API.
| }) | ||
| uiController.registerUiModule(PopupModule, { | ||
| uiController: uiController | ||
| }) |
There was a problem hiding this comment.
You are passing uiController twice - as an argument and as a bind object.
Do you expect, that somehow one uiController will create UIModules for some another UIController?
What would be the workflow then?
There was a problem hiding this comment.
This is a temporary solutions, not part of an architecture. That's for compatibility with legacy code only. The problem is that some UI components that are integrated into a popup (and panel as well) use direct links to a UI controller (which is bad and will be removed after full refactoring is complete). So we have to pass a uiController reference to the popup module to provide it to child UI components.
I've put it as a separate argument to highlight the fact that this is a temporary solution and it should go away eventually. Seeing it passed as an argument serves as a reminder that it should not be there at the end 🙂.
|
I will look closer to the code - but it needs time - from the quick look it is difficult to get an overall picture. |
|
And if to be honest, I didn't understand - what does it mean Are we going to stop using unit tests? Or it would be dependent on developer's choice? |
|
Kirill, it is difficult to see an overall picture - do you have some schemas - What are class dependency links here? Could you describe a simple scenario from methods perspective, for example : |
Sorry, I did not update embedded-lib to work with this version yet. I just wanted to get a feedback on the approach as soon as possible. If it will be approved, I will update embedded lib to work with it. |
We were discussing the unit test approach with @balmas and we agreed that, in general, only public functions of the object should be tested (Bridget, please correct me if I'm wrong here). The idea behind this is that each object can be considered as a black box. It has a public interface: a set of methods that are called by other objects. It also has a private interface: service methods that are called only by methods of the same object. Unfortunately, this separation is mental only right now as it is not supported on the language level, but I hope it will get there finally: https://github.com/tc39/proposal-private-methods#private-methods-and-fields. So far we can start names of private methods and fields with In unit tests we need to verify that an object, being a black box as it is and given some inputs (i.e. have some methods called with certain set of argument values) should provide correct output (i.e. return correct values or produce proper state changes). If only public methods are called from the outside, then we should test public methods only. No matter what other private (internal) methods do and how do they behave exactly: as long as they do not affect the output of the public functions, we do not care (as all we care is the output of the public functions). That will give use some freedom to fiddle with object's internal structure without updating unit tests every time something inside the object is changed (as long as it does not change public methods and their output). Does it make sense? What do you think? |
I was looking at unit tests from another perspective. Unit tests are tests for little parts of the code (that's why they are called unit). And they have several usage purposes:
But as I could see from your text you are using tests only for the last point. |
I will think what diagrams could explain it the best way possible. I should have create one beforhand. Will provide a verbal explanation so far. In
All registered modules are stored in a map ( So we (all references are to the
This code is technical and initialization details might slightly change later. The general idea is that we do three things (in a varying order):
|
That's debatable, that's why I mentioned it 🙂. There are different approaches. I think I'll better put this into a different issue (about our approach to unit tests) as this is probably an off-topic in this PR. I will also remove the last point about testing from the list of advantages just to not concentrate on it here (as it is not critical for the refactoring goals). |
|
Moved discussion of unit testing to alpheios-project/documentation#11 |
|
@irina060981: I've updated description of the module concept in the docs and added a diagram that should explain the whole thing (hopefully) better: https://github.com/alpheios-project/documentation/blob/master/development/app-architecture.md#modules |
|
Thank you for such a detailed explanation, it is becoming much clearer :) Some questions then:
I mean - UIControler -> store All uiComponents will be presented as uiModules - or only root ones?
it is an interesting way to organize code :) oh, also a question - I am using l10n in inflection games and wordList |
Only root components (the ones that create Vue root instance with If inflection games creates a separate popup, then it should have its own UI Module. In this "Inflection Games UI Module" inflection games code should be imported from its separate repository. Also, "Inflection Games UI Module" should crate a root view instance for inflection games in its constructor, and expose Then, in a If a general popup needs to open/close an inflection games popup then the following approach would work best, I believe:
Data modules are just adapter that allow to plug in external libraries (such as inflection games) into a Vue world. The should have no business login inside. Their goal is just to expose public methods and data of an underlying library to the UI components. UI modules (the ones that hold root Vue instances) and UI components (i.e. SFCs) should have no business logic too. Their role is to represent the view in an MVC model. They should have presentational logic only. If they need some data (i.e. to get an inflection table), they use public API methods of an inflection tables data module that redirects request to the inflection tables library. So the general rule is: there should no business logic in anything that is located under the I'm not quite familiar with lexical tests but I think the principles should be:
It can be confusing but data module, UI modules, store module, and public API forms two separate group of entities:
Do you mean in inflection games library or in the inflection games UI (it's important to always draw a clear line between model/view or business and presentational logic)? If L10n is needed inside the library (we're talking business logic here), then an L10n class should be imported there and used as a regular JS object. If L10n is needed by an inflection games UI (presentational logic), then I guess there will be some "Inflection Games UI Component" ( |
|
(moved my comments on testing approaches to alpheios-project/documentation#11) |
|
Maybe a good way to demonstrate the value of the architectural change is the following: We currently pass data to the individual components via the (and the popup component too has a data property, Under the new architecture, if I understand correctly, we would have an InflectionsDataModule and it would expose an api with methods for reading and mutating any state related to inflections data. Any UI component which needs to have access to inflection table data would now interact only with the api methods exposed by the InflectionsDataModule. both Panel.vue and Inflections.vue would do
and then Panel might instantiate Inflections much more simply And in the inflections component, rather than reaching back into Essentially, the DataModules are providing a public api to manage application state for a specific functional area of the system. As we have chosen Vuex as our state engine these are Vuex modules. But if at some point we switched to a different state engine, the pattern theoretically would still work. Instead of the Data Module using a Vuex $store object for state mutations, it would use something else, but the public api of the data module might not need to change at all. This should hopefully also make testing much cleaner. Right now we have to go through alot of work to wire up the properties of the Panel and Popup Vue components for our tests, and when something changes with those data structures everything breaks. Instead we should be able to just create Mock objects which implement the public apis of the data modules and use those in both unit and integration tests as needed. @kirlat is this an accurate description? |
|
As far as the UI Modules are concerned, I believe these are primarily intended to make it easier for different application implementations to reuse composite components and not about state management. |
There was a problem hiding this comment.
I'm generally fine with this, but because I agree with @irina060981 that we need to be pushing ourselves to do more test-driven development, would you mind adding a unit test for the L10NModule before merging the PR? Thanks! (Also for some reason, the CI tests are failing on one of the two passes. Can you see if you can fix that too?)
|
Thank you for such a detailed explanation you both, @kirlat and @balmas . But for now - I like this approach, I like the independence that it could give , like the organization of the code approach here. |
|
@irina060981, @balmas: There are some very important points and ideas in this discussion, and a decision about a component's architectural change is pivotal and significant one. Because of this, it would be important that we dot the i's and cross the t's before make the architecture final. So if we could continue discussing an architecture for a couple more days more to make sure we've made all the best decisions here, it will be super beneficial for the project overall, on my opinion. There are some important decisions we're making we need to be clear about.
It's also important that DataModules, in addition to the public API, provide state retention. Libraries are usually stateless: if we make request to inflection tables library, it returns inflection tables for the word provided. Library does not store those tables anywhere after request is processed. However, we do need to keep those tables somewhere while they are displayed in the UI. The thing that is bugging me is that Vuex store module and public API do have, on my opinion, an overlapping functionality and we could, in theory, eliminate one or the other, making things simpler. Let me explain why is it this way and what are some possible other ways to implement an architecture. Let's start with Vuex store modules. It's a standard way to store and distribute state in a Vue ecosystem. In theory, we could use it for all app state management. However, there are several important issues with this approach (please correct me if I'm wrong or if you do not agree):
If we could accept that all data would be reactive only (and accept performance and memory consequences it might have) and agree to use Vuex actions as regular methods for everything we do, we could stick with Vuex only. No need for the public API. Things get simpler. The public API: if we sacrifice data reactivity, we could use public API only and eliminate Vuex completely (and thus reduce our dependency on third-party tech). If we decide to go with it, all data can be stored inside a data module instance. Of course, such data is not reactive. Because of this, we have to use public API functions whenever we want to access it. So instead of accessing There are several obvious drawback to this approach:
The rest are all advantages, on my opinion:
So we could, in theory, use either Vuex store modules OR data modules with its public API. That will make things much simpler, but there are some sacrifices involved. What is your opinion on that? |
This is correct. UI modules group subcomponents together to form a self-sufficient UI unit that can be reused in different apps. One can think of a UI module as of some complete real life object (i.e. a car or a house) built out of LEGO bricks (i.e. UI components aka SFCs). One can always build anything out of a set of LEGO bricks, but sometimes it is convenient to keep something assembled so one can take it and play with it right away 🙂. Another important purpose of UI modules is that they hold the Vue root instance object (created with |
|
@balmas, @irina060981: Here is another important thing I would like to discuss:
Traditionally, we started with storing data divided by functional areas. Then we used some logic inside UI components to assemble pieces from different areas together. That's because data was tightly coupled with libraries that produced it: inflection tables data was updating inflection views when asked to do so without knowing what happens with all other data requests. However, it creates conceptual problems similar to described above (please correct me if I'm wrong here). These problems will grow once relationships between different data pieces become more complex. Also, a need to store user history and all related data puts additional pressure to it. At any given moment of time different pieces of data can be in a different state and we have to synchronize it with some (sometimes) non-trivial logic. Let's say there was a lookup request for one word and then a new one for the other. So for each new request we have to manually clean inflection views, morphology data, notifications etc. When new data arrives step by step we need to remember to put its pieces to some different places of storage and not to forget notify other places about its update. That's messy and error prone. I'm wondering if there is a better way to handle this? What if we decouple data from its libraries and group it all under a higher-level entity? Users accesses data word by word. They start from one word, then goes to the other. They may want to return to a previous word sometimes. We need to store a single word with all its related data in history. What if we create a word (not a real name, but a conceptual term) object to keep all word-related data? If user starts a new word lookup, we create a new word object that is empty initially, but starts to fill in with data once more information comes in. In the UI, we just replace an old word object with the new one, and all data that UI displays changes with it automatically. The old word object can be destroyed, or stored into history (to indexedDB or remotely) and destroyed after that. Clean and simple. No more tedious and error-prone data cleaning (https://github.com/alpheios-project/components/blob/master/src/lib/controllers/ui-controller.js#L436-L443) (if we add a new piece of data shall we not forget to add its cleaning to the What do you think about this concept? Would that be better than what we have now? |
|
Kirill, do you expect to store and track all the data that was created during user interactions in VUEX? And about LexicalQuery - it is used from different piecies of the code - from UIController and from embed-lib for example, or from command-line tool. |
In the current implementation it is mixed: each module has its own instance props that are not reactive (see for example https://github.com/alpheios-project/components/blob/master/src/vue/vuex-modules/data/l10n-module.js#L10-L13) and the So if we need to store some data that should not be reactive, we should put it to the non-reactive part of data storage (to the data module's props). In order to do that, we have to use methods of the module's public API. But in the #318 (comment) I'm wondering if such dual approach is an overkill.
If table is displayed in the UI and this is part of the store's reactive data, then yes, it is (I'm a supporter of a Vuex's strict mode approach: https://vuex.vuejs.org/guide/strict.html). But that:
If I understand you correctly, no. LexicalQuery is a "regular" JS data object. It's not exposed to the Vue world (it's not a data module or anything). It has no data exposed to the store, and no Vuex mutations or actions should be used with it. So other pieces of code should use it the same way as they did before. |
I am talking about this piece of your description:
|
In that case a The difference between a LexicalQuery and a "Lexical Query Data Module" is that a LexicalQuery is stateless. It keeps no data after lexical query is complete. It works well for libraries (they store lexical query state on their own), but it does not work for Vue templates, where we have to provide data objects to the template engine (we can store data in data props of UI components but then it cannot be shared; it will lead to duplication of data). That's why there is a need for a "Lexical Query Data Module", that encapsulates LexicalQuery methods (exposed via a public API) PLUS provides state retention (it keeps results of the last lexical query until the next query overwrites it). If we decide that a global "word" state object (#318 (comment)) will be beneficial, we could shift all state management there. Then "Lexical Query Data Module" could become stateless, or even there might be no need for "Lexical Query Data Module" at all (it could be just a lightweight wrapper that exposes LexicalQuery methods, maybe even not as a separate object, but as in interface within a UI controller). The change of architecture of components is going to be a big one. It will require an effort. Bit if done right, it can bring many tangible benefits. If we start doing it, we should strive to do it right. That's why I think we should analyze every possibility. Only then we would come up with solutions that will satisfy all our requirements the best way possible. |
|
Summarizing the results of a Slack discussion between myself and @kirlat : 1. Word object: this suggestion aligns closely with the work @irina060981 and I are doing on the WordList, WordItem and WordListController for the user word lists feature. It is in the separate wordlist repository right now, but will be moving back into the core data-models and components soon. A WIP class diagram: In this design, the wordlist controller subscribes to LexicalQuery events and populates the word object as they are received, itself issuing events that the the UserDBManager then subscribes to update the corresponding remote and local stores. The WordItem object is intended to be an aggregation of all instances of a user looking up that word in their history of using the application, so it might not be the same as a Word object that is aggregating data related to application state, but there is clearly some overlap here and the design is probably converging on the need to have a central data module for for this. We will revisit this in more detail soon when the WordList design work is done. 2. Organization of State: we generally agreed that it would be beneficial to group state into conceptual layers:
3. Use of Vuex actions: it may be okay to use these for things that impact UI state but we probably should proceed with some degree of caution 4. api methods vs direct store access: we have decided to keep the api concept but limit for now the api methods to things which cannot be achieved via direct store access. So a DataModule becomes an encapsulation of a set of functionality that needs to be made available to the UI. It may declare state data for the Vuex store and it may declare api methods, or it could declare only one or the other. The UIController will gather the functionality it needs by registering the specific functional modules. Modules can be injected into the individual Vue components to provide them with functionality. The Vue components will interact directly with the reactive state properties injected into the Vuex store, and with the api methods as needed. 5. Non-reactive data: we will use properties of a data module instance to store non-reactive data |
…dated dependencies.
…ed some helper functions to L10n. Updated tests
# Conflicts: # dist/alpheios-components.js # dist/alpheios-components.js.map # dist/alpheios-components.min.js # dist/style/style.css # dist/style/style.css.map # dist/style/style.min.css # package-lock.json # src/lib/controllers/ui-controller.js # src/vue/components/panel.vue
|
I'm merging this after integrating all upstream changes |
Here is a first take on a modular UI concept. It is not final yet, and has many imperfections. For example, there are multiple hard links to a UI controller or other components that are planned to be removed later. That's for compatibility with the existing code base. It's just an extremely involving task to change all it once. It is very error prone, too.
So I would like to offer this proof of concept for discussion first. If we decide that it's developing in the right direction, we can continue forward with it. Otherwise, we can make amends or even reverse a direction if anything is wrong with the approach.
The core concept is: all modules in a UI are dynamic. They are registered dynamically. There is no hard links between them. If module needs some other ones to function, it will check it during initialization and print a message if the check fails. That adds a safety net into a dynamic environment.
There are two major pieces that need to be shared among modules. The first one is data, and it is shared via a Vuex store modules. Each module has (usually) its own store object that's integrated into a global store. The second one is groups of functions, a public API of a module. Not all actions fit well into Vuex concept of actions and mutations. Sometimes we need to do something that is not related directly to the Vuex store data. That's what public API methods are for.
There are two types of modules: data modules (they are adapters to the libraries) and UI modules. Each UI module represents a single independent UI entity, such as a panel or a popup. All modules are registered dynamically in the UI controller's
create()function. This allows a client of a UI controller (e.g. embedded lib) to create its own configuration of modules.All registered modules are instantiated by a UI controller during an
initphase. Data modules are instantiated first, the goes UI modules. That's because data modules do not have dependency on the UI modules, but the UI modules do.During initialization a UI controller creates an instance of each module with (optional) parameters that were specified during module registration. It also inserts module's store object into a global store. All store modules are namespaced.
Each module declares its public API within its
apiobject. UI controller does too as it has some methods that it would like to share with UI modules. Those API objects (i.e. groups of methods) are mounted into the root Vue instances withprovideoption. With this, all those methods becomes available to any child UI components that declare their use withinject. This allows to clearly specify what component depends on which module.That's the concept in general. In the current implementation there is one data module (l10n) and two UI modules (popup and panel). Each of UI modules uses
visibleprop from the Vuex store and UI controller provides methods to change visibility of a popup or panel (so this can be done outside of the popup or panel).Please refer to the docs for some more information on the module concept: https://github.com/alpheios-project/documentation/blob/master/development/app-architecture.md#modules
The concept of a public API is, on my opinion, important because it:
inject(for API groups) orstoreModulescustom prop (for store modules).Please let me know what do you think. Would really appreciate your feedback! It took some time to come up with the concept, but I think it's starting to become consistent and in line with our requirements and the Vue ecosystem. Thanks!