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.

Sign up for GitHub

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

Jump to bottom

bug with module inheritance #20

Open
Disorrder opened this issue Jun 25, 2019 · 9 comments
Open

bug with module inheritance #20

Disorrder opened this issue Jun 25, 2019 · 9 comments
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@Disorrder
Copy link

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:

vuex-class-modules/src/actions.ts

Line 9 in 2dab4e7

if (!vuexModule.__actions) {

Child classes inherit this vuexModule.__actions property, that"s why Roads" methods overwrite Projects" one.
@gertqin
Copy link
Owner

gertqin commented Jul 3, 2019

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
Copy link
Author

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

@bodograumann bodograumann added the bug Something isn't working label Oct 10, 2019
@bodograumann
Copy link
Collaborator

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 bodograumann added the help wanted Extra attention is needed label Oct 10, 2019
@FullPint
Copy link

@Disorrder

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

@seflue
Copy link

seflue commented Aug 30, 2020 •
edited
Loading

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
Copy link

seflue commented Aug 30, 2020

Ok, I just send you a minimal fix.

@gertqin
Copy link
Owner

gertqin commented Oct 3, 2020

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
Copy link

seflue commented Oct 3, 2020

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.

gertqin added a commit that referenced this issue Oct 4, 2020
@gertqin
Merge pull request #44 from seflue/minimal_fix_issue_20
e54f345 
Fix inheritance for actions and mutations (#20)
@seflue
Copy link

seflue commented Oct 5, 2020 •
edited
Loading

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

@bodograumann bodograumann mentioned this issue Jul 13, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@seflue @bodograumann @Disorrder @FullPint @gertqin