vuex-class-modules icon indicating copy to clipboard operation
vuex-class-modules copied to clipboard

bug with module inheritance

Open Disorrder opened this issue 5 years ago • 9 comments

Trying to use this module for share common logic Found question #13 and it works, but with bug of initializing actions (and maybe mutations too)


export default class Collection extends VuexModule {
    url = "/collection";
    items = [];

    @Action async getList() {
        //...
    }
}

@Module
export default class Projects extends Collection {
    url = "/project";
    @Action async getList() {
        // change something
    }
}

@Module
export default class Roads extends Collection {
    url = "/road";
    @Action async getList() {
        // change something else
    }
}

// init modules
import Projects from "./Projects";
export const projects = new Projects({ store, name: "projects" });
import Roads from "./Roads";
export const roads = new Roads({ store, name: "roads" });

And when I call projects/getList it calls method from roads.

I think problem is here: https://github.com/gertqin/vuex-class-modules/blob/2dab4e763d203ed16d348ee99204008c608aa9a1/src/actions.ts#L9

Child classes inherit this vuexModule.__actions property, that's why Roads' methods overwrite Projects' one.

Disorrder avatar Jun 25 '19 18:06 Disorrder

Looks like the actions are stored in the base class instead of the derived class, which definately is a problem for inheritance.

I'll take a look at it when I get the time.

gertqin avatar Jul 03 '19 18:07 gertqin

@gertqin Okay, that's would be nice. I can't wait, so I wrote my own decorators :D. Maybe upload to github soon

Disorrder avatar Jul 03 '19 18:07 Disorrder

The problem here is that vuex modules don’t have any concept of inheritance. For helper methods, we only call the inherited instances with the correct this. When looking at actions, mutations and getters though, they have to be transformed into the vuex objects. So at that point the inheritance vanishes. That means we have to do some unrolling. Currently this is done (maybe by accident), through the inheritance of __actions etc.

My idea to solve this is to make __actions private and manually combine all of the objects from the prototype chain.

bodograumann avatar Oct 10 '19 13:10 bodograumann

@Disorrder

Why not either create a generic base class, or use a constructor?

FullPint avatar May 26 '20 19:05 FullPint

Hi. I sat down and implemented a fix. Basically the solution was very simple (after I figured out, what really is going on - which took some time): treat the keys of __actions and __mutations just as label storage to decide, if a module property is actually an action or mutation and do the assignment in the same manner (and even the same loop) as the getters and helper functions.

Beside the fix I did some version bumping, small improvements about readability and a bit refactoring around the mentioned loop. My question @bodograumann or @gertqin is: Would you like to have a pull request with the minimum fix (then I have to do some more work) or would you accept a pull request with all these things together (certainly split up in several commits)?

seflue avatar Aug 30 '20 18:08 seflue

Ok, I just send you a minimal fix.

seflue avatar Aug 30 '20 20:08 seflue

Hi seflue, sorry for my late reply! I have been busy with other stuff (parental leave :)), so I haven't really been at a computer the last month.

Thanks a lot for your help, I have taken a short look at it, and so far it looks good! Any improvements are welcome, including version bumping and refactoring/readability improvements (although this is somewhat subjective, so I might not agree with everything :p), so if it is not too much extra work for you, feel free to commit your other improvements, and I'll take a look at it :)

gertqin avatar Oct 03 '20 07:10 gertqin

If you don't mind, it would be the easiest for me and my colleagues, if you just merge the PR first and for further improvements I will create separate PRs.

seflue avatar Oct 03 '20 23:10 seflue

@gertqin We should bump up the version to 1.1.3. I forgot that in my PR.

seflue avatar Oct 05 '20 07:10 seflue