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

Have options independent of module #5

Closed
bodograumann opened this issue Jan 11, 2019 · 3 comments
Closed

Have options independent of module #5

bodograumann opened this issue Jan 11, 2019 · 3 comments

Comments

@bodograumann
Copy link
Collaborator

Currently the options: RegisterOptions need to contain a name. So if I want to use dependency injection, I will have to inject a different value into each module class.

For my usage of vuex-class-modules, where I only use the proxy objects directly, the name parameter is not important, apart from having to generate a unique vuex module. The best solution would of course be to just have Symbol() as the module namespace, but that is completely incompatible with vuex I think.

So my suggestion would be to either

  • allow the name option to be left out. Then, if it is not provided, generate a unique random name.
  • Or allow name to be a function, which is called to generate the name. Then the user can decide for themself whether to use a random or deterministic name.

Another thought is to somehow use the class name when generating the vuex module name.

@bodograumann
Copy link
Collaborator Author

The simplest implementation would be having VuexModule as:

export class VuexModule {
  private __options: RegisterOptions;
  constructor(options: RegisterOptions | ((className: string) => RegisterOptions)) {
    if (typeof options === "function") {
      options = options(this.constructor.name);
    }
    this.__options = options;
  }
}

The problem with using this.constructor.name is that in the minified version it will be mangled. Maybe multiple different classes might even get the same name then⁉

@gertqin
Copy link
Owner

gertqin commented Jan 11, 2019

I don't see any use cases other than DI where you wouldn't want to add a name when registering the modules (for instance in Vue Devtools you would like to be able to identify the modules). Vuex' store.registerModule also requires a name (path).

As you mention, using constructor.name has the problem that it will be mangled when minified, and I'm not sure the names will be unique, so I don't like this solution.

For your need, can't you just change the constructor of your modules to something like:

@Module
class MyModule extends VuexModule {
  constructor(store: Store) {
    super({ store, name: "myModule" })
  }
}

?

@gertqin gertqin closed this as completed Jan 11, 2019
@bodograumann
Copy link
Collaborator Author

This might not pose an immediate problem, but it goes against the principles of dependency injection, because then every module must make sure not to collide with any other.
When I said, using the module class, I meant “based on the module class”. So some uid could be added.

Anyway, I can of course define my own constructor, which allows the options factory and calls it before invoking super. So it’s ok that this is not changed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants