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

Feature as controller #5262

Closed
pjasiun opened this issue Oct 19, 2016 · 2 comments
Closed

Feature as controller #5262

pjasiun opened this issue Oct 19, 2016 · 2 comments
Labels
package:ui type:improvement This issue reports a possible enhancement of an existing feature.
Milestone

Comments

@pjasiun
Copy link

pjasiun commented Oct 19, 2016

tl;dr: UI MVC is overcomplicated. It should be possible to create the whole code of the controller inside the feature.

Here is my use case:

I was writing a feature which takes a collection of users and displays the content of it (assume that I already have a User and Users model classes). I did not want to make it reusable.

I created UsersFeature, UsersView, UserView, UsersController and UsersController and had a feeling that it's overengineered. Also, all my controllers do is:

export default class User extends Controller {
    constructor( model, view ) {
        super( model, view );

        view.bind( 'name' ).to( model );
    }
}

And:

export default class Users extends Controller {
    constructor( model, view ) {
        super( model, view );

        this.addCollection( 'users' );
    }
}

Also, I realized that there are many such controllers in the code (both in my project and in the UI default lib).

I understand that in the UI default lib which contains reusable component it makes sense to create such structures, but in my case it was useless.

Moreover, I understand the controller as a piece of the code which binds model and view, handles the initialization and the data transformation between the model and the view.
According to this definition, It is not a real controller, because the whole code which was handling initialization and binding model with the view was in the feature.

Then, @Reinmar told me that I can avoid creating these classes controller by creating Controller class and do it this way:

const usersCotroller = new Controller( usersModel, usersView );
usersCotroller.addCollection( 'users' );

usersCotroller.collections.get( 'users' ).bind( usersModel ).as( ( userModel ) => {
    const userView = new UserView( locale );
    userView.bind( 'name' ).to( user );
    return new Controller( userModel, userView );
} );

Still, I see 2 problems here:

  • I need to create instances of Controller without the clear understanding what it does,
  • I need to add a collection to the first controller (usersModel has a collection of userModels and usersView has a collection of userViews, why I need add to the usersController collection of userControllers?).

I would like to write in the UsersFeature, which is a real controller, code which binds model and view without a need of creating additional Controllers. bind could be a method of the collection and I should be able to do:

usersView.users.bind( usersModel ).as( ( userModel ) => {
    const userView = new UserView( locale );
    userView.bind( 'name' ).to( user );
    return userView;
} );

As I learned the idea behind controllers was that they will handle destruction chain and prevent memory leaks. The problem is, that in this case, they are the reason for the memory leak. Before writing this post I did not realize that I have to store usersCotroller as a private property and call usersCotroller.destroy() in the feature destroy() function.

@pjasiun pjasiun changed the title Controller vs feature Feature as controller Oct 19, 2016
@pjasiun
Copy link
Author

pjasiun commented Oct 19, 2016

In fact, thinking about it in the more generic way, my point is that Controller should be a Controller which helps you creating a controller, not the only Controller you need to use to bind view and model. Part of its functionality should be moved as tools which you can use separately. The same way that there is a Model class you can use as a base of your class, but any class which mix Obseravbles or even fire proper events can be a model.

@Reinmar
Copy link
Member

Reinmar commented Nov 9, 2016

Fixed by ckeditor/ckeditor5-ui#95.

@Reinmar Reinmar closed this as completed Nov 9, 2016
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-ui Oct 9, 2019
@mlewand mlewand added this to the iteration 5 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:improvement This issue reports a possible enhancement of an existing feature. package:ui labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:ui type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

No branches or pull requests

3 participants