gertqin / vuex-class-modules

Typescript class decorators for vuex modules
MIT License
193 stars 20 forks source link

bug with module inheritance #20

Open Disorrder opened 5 years ago

Disorrder commented 5 years ago

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.

gertqin commented 5 years ago

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.

Disorrder commented 5 years ago

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

bodograumann commented 4 years ago

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.

FullPint commented 4 years ago

@Disorrder

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

seflue commented 4 years ago

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 commented 4 years ago

Ok, I just send you a minimal fix.

gertqin commented 3 years ago

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 :)

seflue commented 3 years ago

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 commented 3 years ago

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