-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Why do we need view models? #5251
Comments
OK, I can name one argument for separating the model – clarity of what has to be defined (bound by the controller) in order to render that specific view. The cons are quite obvious – additional property, additional interfaces, etc. |
I think, I understand the concept, but I have a strong feeling that it is a class interface what should be... an interface.
Still, there are simpler ways to define that the interface should contain: base class or simply a documentation. We do not need nested property as a public interface. It looks like the reversed facade pattern ;) |
Eh... It's so obvious now, but somehow it wasn't when we were introducing VMs. I think that we set correct requirements, but we failed at realising what's the easiest solution. Anyway, it should be a fairly easy change to do – nothing serious to refactor. Just many automatic changes in the tests and docs. However, as we're discussing this now – let's take a look at what we'll end up with, to reassure that we're going in the right direction. So, a controller has a model. It is passed to the controlled by the "world" (so multiple instances can share one model) and that model is binded to the view (currently it's binded to the view model). So a model is component's API, while view defines its API itself. I know that there are multiple ideas how MVC should be structured, so I hope that this approach is understandable as well. In terms of whether it's going to work for us, we can be pretty sure that it will because it's just a small change comparing to what we have now. |
It looks like the most classic MVC approach for me, what is nice and easy to understand. |
One more thing – the result of the current architecture is that our views can contain their runtime state which doesn't have to be exposed in the component model (because it's not a business data of the component). E.g. a multiple drop-down components can share one model and only their respective views will be able to tell you whether a specific drop-down is opened. In such case other controllers have to access those views to change that state. That's a bit similar to CSS where you have It may, however, be controversial that we divide the state into business and view layers. It is also unclear what should go where. In the drop-down we assumed that "isOpen" goes to the view (and it's unlikely that anyone will need to use those), but how to open a balloon panel (balloon panel is usually used directly by a feature)? Also through the view or should this state be exposed in the model? |
There's a third option – through a controller's methods which then change the view. |
I didn't know that view model should be the only view interface, I thought it is for storing specific for view, observable data which can be bound to the template. But I didn't know that I couldn't extend view API by adding methods.
For me it sounds good, but I don't think that model has to be the only interface.
It sounds like MVC. If something is only a matter of view it should stay in the view.
I was expecting that. It seems to be the easiest way to use components. const balloonPanel = new BalloonPanel( ... );
balloonPanel.show(); // And I don't have to know component architecture. |
There were multiple reason behind separate InterchangeabilityThe first one was interchangeability, which is that each We wanted to have a such architecture that would allow switching all the views to a different library by a simple injection in the compiler (builder). For instance, as the default library (ckeditor5-ui-default) defines In fact, it's been a long, long time since we tested such process (and it worked). I wonder if it's still possible. UI duplicationAnother reason for separate VM is reflected in the So to have 2 components sharing the same application logic (like heading formats) but separate view states, there must be a separate VM for each of the No magic in component definitionWhen you define a component, you provide its model, const dropdownModel = new Model( {
logicAttr: 'foo',
anotherLogicAttr: 'bar',
} );
const dropdown = new Dropdown( dropdownModel, new DropdownView() );
console.log( dropdownModel );
// logicAttr: 'foo',
// anotherLogicAttr: 'bar', The Now, if suddenly there was no const dropdownModel = new Model( {
logicAttr: 'foo',
anotherLogicAttr: 'bar',
} );
const dropdown = new Dropdown( dropdownModel, new DropdownView() );
console.log( dropdownModel );
// logicAttr: 'foo',
// anotherLogicAttr: 'bar',
// isOpen: false <---------------------- Where did that come from, huh? I didn't define it. WTF? so the developers would face a situation that the models they create are automagically extended by some attributes of unknown origin and purpose. It could be a problem because such model becomes unpredictable and there's a lot of garbage if it's to be serialized, because some attribute like |
Me neither, and I coded it :P It makes sense that Please note guys that ATM many |
The VM could be seen as the container that holds the essential data that drives the View behavior (like a serialization of the View). User interaction on the View is then converted into data changes and whichever change to that data, usually coming from the controller, is reflected in the View. Views should certainly be able to introduce a custom API with own methods to handle its behavior or even semantically rich shortcuts to manipulate the VM, in a clear View PoV... for example, things like VMs can have some "dumb" logic, if, for example, a change in one of its attributes results on logic to update other attributes. Nothing more than that though as VMs know nothing about the View nor about the Controller. |
Our view model looks like nothing more as namespace with observable properties. It is not injectable, can't be shared between views, it is just a namespace. Question is do we need namespace for storing properties in the class? |
One of the essential features MVC features, in fact, is that a View can receive its Model in the constructor... this is much probably the part that messed things up in our implementation as we don't allow that. |
I think this could be achieved with the class interface.
I think that the key question here is: if we need view serialization? In my opinion, it is the model what should be serialized and view should be re-created based on the deserialized model. Then that view-only features link "if the dropdown is open" will not be recreated, only these which are based on the model state: for instance balloon panel might be recreated based on the selection. I do not think that view should be serialised, that anybody expects to have open dropdown after deserialization, but I might not know about some cases we want to handle. |
Also, as I think about it: we do not create some special property in the model to help with serialization. There is no |
Can you elaborate, @pjasiun ? As for
when writing
I meant component (Controller) model, not the VM. My message was that if Views share the same model with Controllers, then the model of the controller may get polluted by unexpected stuff created by the View. And it's the model of the controller which is to be serialised, if any. |
@oleq pointed out all reasons why we thought that separating VM will be reasonable. However, merging VM into the V won't change anything else than paths and number of interfaces. Currently, each component instance has a duet of V+VM (they always go together). After the change, each component instance will have an instance of its view.
That couldn't have worked with VMs – we tried that. Such component initialisation was a total mess. And MVC says nothing about how model is connected to the view.
That's basically how we see this today, but there's no reason why we have to stick to that model. The discussion here is about simplifying this model, so there's less code and not talk about VM, which is confusing. Of course, I agree that by merging things together we may also introduce some confusion in more complex situations, but I haven't seen yet a model which wouldn't cause any WTFs. So, in this case, I'd prefer simplicity over explicitness, because we don't lose any functionality at the same time. |
Alright. I'm for this refactoring, which is merging VMs in to their respective Vs. It should not be a big thing, in fact, it's more about copy and paste than actual coding. i3? |
Sure. I mean: mix( View, ObservableMixin ); export default class DropdownView extends View {
constructor( model, view ) {
super( model, view );
this.set( 'isOpen', false );
this.addCollection( 'content' );
}
} Plus we could have a Docs which describe that every export default class DropdownView extends DropdownViewApi But the Then, as @oskarwrobel mention, we could provide nice API in the controller, for other plugins: export default class Dropdown extends Controller {
show() {
this.view.isOpen = true;
}
hide() {
this.view.isOpen = false;
}
} |
Since the scope of the refactoring is known, I decided to move it to iteration 4 because it's likely to take a couple of days to code it and i3 has already lots of pending tasks. Besides, no new components will arrive before i4 so the scope remains the same. |
OK, I hate to say that, but I don't remember or I don't know the answer. Either way, it sounds like we overcomplicated this thing...
So, currently we say – the view model (created automatically by each view and used by controllers to push data into the views and get the feedback) is the view's interface. It's how the world communicates with the view.
That's even fine, but:
attachTo()
– I guess its params should be put in the view model...).init()
anddestroy()
plus couple of other things, but still... why moving the rest outside?The text was updated successfully, but these errors were encountered: