Skip to content

add hasModule API #833

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

Closed
FranckFreiburger opened this issue Jun 13, 2017 · 37 comments
Closed

add hasModule API #833

FranckFreiburger opened this issue Jun 13, 2017 · 37 comments
Labels

Comments

@FranckFreiburger
Copy link
Contributor

What problem does this feature solve?

When adding/removing modules dynamically, it is sometimes mandatory to know if a module has already been added.

What does the proposed API look like?

Store#hasModule(path)

@nickmessing
Copy link
Member

You can wait for Optional Chaining Operator to get into babel and use that instead. Looks like ES Committee likes the idea and is going to be discussed at the next meeting again.

@FranckFreiburger
Copy link
Contributor Author

I am talking about vuex store modules

@posva
Copy link
Member

posva commented Jun 15, 2017

But if you add or remove modules dynamically, you should be able to add a property like added to the module and set it as you need it.
Why would your code add the same module twice?

@FranckFreiburger
Copy link
Contributor Author

FranckFreiburger commented Jun 15, 2017

I try to make any Component to support ssr (not only route components).
These components may use the global store to create their own namespace (module).
On the server side, there is no problem since asyncData() is called only once by component by rendering.
However, on the client-side, the asyncData may be called several times. If the component want to create it's own vuex module in the asyncData, it should check if the module has not been previously registered.

eg. with hypothetical Store#hasModule(path)

export default {
	name: 'Test',
	asyncData: async function({ store, route }) {

		if ( !store.hasModule('testVuexModule') ) {
			
			store.registerModule('test', {
				namespaced: true,
				mutations: {
					'TEST': function(state, value) {
						state.testVal = value;
					}
				}
			})
		}

		store.commit('test/TEST', await api.call(...))

@posva
Copy link
Member

posva commented Jun 15, 2017

I see. Still, adding a custom property to the module should work to check if it has been installed

@FranckFreiburger
Copy link
Contributor Author

FranckFreiburger commented Jun 15, 2017

yes, but who holds the reference to the module ?
the store is the only one that is able to do that, no ?

(Furthermore, it is quite common to have foo.add(), foo.remove() and also foo.has(), imagine a world without the javascript in operator ...)

@posva
Copy link
Member

posva commented Jun 15, 2017

This is what I'm saying:

import module from './module'

if (!module.registered) store.registerModule('m', module)
module.registered = true

The has or similar exists in arrays and collection because they're general purpose collections

@FranckFreiburger
Copy link
Contributor Author

ok, but this will break my progressive app, since this code will also run server-side

@posva
Copy link
Member

posva commented Jun 15, 2017

why?

@FranckFreiburger
Copy link
Contributor Author

FranckFreiburger commented Jun 15, 2017

server-side, module.registered will always be true (except the first time of course)

@posva
Copy link
Member

posva commented Jun 15, 2017

Well, isn't that the point? Registering only once?

@FranckFreiburger
Copy link
Contributor Author

yes

@posva
Copy link
Member

posva commented Jun 15, 2017

@ktsn Does it makes sense to add a module twice?
Vue plugins add a property to know if they have been installed and prevent installing again. We could maybe add that too

@FranckFreiburger
Copy link
Contributor Author

(FYI, see PR #834)

@ktsn
Copy link
Member

ktsn commented Jun 15, 2017

@posva Adding registered flag seems smarter solution. 👍

I'm not sure why we still need hasModule while we could achieve the original goal by adding the flag?

@FranckFreiburger
Copy link
Contributor Author

As explained in previous comments, on server-side rendering, modules are shared between all sessions

@posva
Copy link
Member

posva commented Jun 19, 2017

I don't think we're in sync here... Wasn't that the point? Do you need to run every request in a new context, at least by user or similar?

@FranckFreiburger
Copy link
Contributor Author

On the server-side (ssr), I process my requests in the same context (runInNewContext: false) this mean that every vuex modules definition will be shared between all the users that access to my application. If I set a flag in the module (eg. registered = true), this flag will be true even it the module has not been registered !
The only "thing" that is able to tell me if a module is registered for a given session is vuex. Unfortunately there is not API (at the moment) to ask this information, this is why I suggest to add Store#hasModule(path) or better: Store#isModuleRegistered(path)

@FranckFreiburger
Copy link
Contributor Author

As you supposed before, the point is: Registering only once by vuex instance, that is equivalent to:

if ( !store.isModuleRegistered('foo') ) {
  store.registerModule('foo', ...)
}

@ktsn
Copy link
Member

ktsn commented Jun 25, 2017

I'm actually not sure about SSR but it makes sense for me. But wouldn't it be better to add like ignoreIfExists option to registerModule if you just want to check the existence for adding the module? Because it is a little verbose and procedural to exec both hasModule and registerModule.

@FranckFreiburger
Copy link
Contributor Author

It is true that ignoreIfExists option is less verbose than hasModule + registerModule, however, hasModule allow to check module existence before defining it, and this may save performance in the case where initial module data is not static, eg.

store.registerModule('Foo', {
        ignoreIfExists: true,
	namespaced: true,
        state: await getInitialFooStateFromASlowSource(),
	mutations: ...
})

In this case, the module will be completely setup but never used (except the first time)

@ktsn
Copy link
Member

ktsn commented Jul 5, 2017

Makes sense. Let's move on your PR 🙂

@anteriovieira
Copy link

Hi @FranckFreiburger , what was your workaround?

@FranckFreiburger
Copy link
Contributor Author

Hi,
currently I use the following code as workaround:

if ( !Vuex.Store.prototype.hasModule ) {

	const _registerModule = Vuex.Store.prototype.registerModule;

	Vuex.Store.prototype.registerModule = function(path, rawModule) {
		
		if ( !this._registeredModules )
			this._registeredModules = new Set();
		this._registeredModules.add(path);

		return _registerModule.call(this, path, rawModule)	
	}

	Vuex.Store.prototype.hasModule = function(path) {

		if ( this._registeredModules )
			return this._registeredModules.has(path);
		return false;
	}
}

Note that is is not polished/optimized, it's just a workaround.
Alternatively, Deep checking $app.$store._modules.root._children ... could also work

@FranckFreiburger
Copy link
Contributor Author

ping!

@Codermar
Copy link

Has this proposal gained any traction?

maoberlehner added a commit to maoberlehner/how-to-structure-a-complex-vuex-store that referenced this issue Mar 31, 2018
It is necessary to check private (prefixed with _) properties of the
Vuex store, because currently there is no other way of how to check if a
component was already registered. See
vuejs/vuex#833

Fixes #1
@oceangravity
Copy link

This works for me:

if(!me.$store._modules.root._children[`module-name`]) {
    me.$store.registerModule(`module-name`, {
        ...        
    });
}

@martynchamberlin
Copy link

martynchamberlin commented Aug 9, 2018

Bahaha I'm with @jcgalindo. The data is definitely there, exactly as he describes, but it's definitely an obtuse data structure. Feels like we're writing Java or something. Oh well, it gets the job done for now.

Interestingly you could also do this, which is a little less verbose but should be every bit as reliable:

if(!me.$store.state[`module-name`]) {
    me.$store.registerModule(`module-name`, {
        ...        
    });
}

@KRaymundus
Copy link

KRaymundus commented Nov 1, 2018

I have extended the good solution of @FranckFreiburger to support nested namespaces:

const storeProto = Vuex.Store.prototype
if ( !storeProto.hasModule ) {
  const registerModuleOriginal = storeProto.registerModule

  function pathToKey(path) {
    return path instanceof Array ? path.join('/') : path
  }

  storeProto.registerModule = function(path, rawModule, opts) {
    if (!this._registeredModules) this._registeredModules = new Set()

    let pathKey = pathToKey(path)
    this._registeredModules.add(pathKey)

    return registerModuleOriginal.call(this, path, rawModule, opts)
  }

  storeProto.registerModuleOnce = function(path, rawModule, opts) {
    if (this.hasModule(path)) return false
    return this.registerModule.call(this, path, rawModule, opts)
  }

  storeProto.hasModule = function(path) {
    let pathKey = pathToKey(path)
    if (this._registeredModules) return this._registeredModules.has(pathKey)
    return false
  }
}

Deep checking $app.$store._modules.root._children is more likely to break if Vuex evolves, and it get's complex with nested namespaces.


Edit: this doesn't work if a module wasn't registered through this extension. Using the answer below yields this:

let storeProto = Vuex.Store.prototype

storeProto.registerModuleOnce = function(path, rawModule, opts) {
  if (this.hasModule(path)) return false
  return this.registerModule(path, rawModule, opts)
}

// Path can be an array or a string: ['my', 'nested', 'module'] or 'my/nested/module'
storeProto.hasModule = function(path) {
  let pathArray = path instanceof Array ? path : path.split('/')
  let m = this._modules.root
  return pathArray.every(p => {
    m = m._children[p]
    return m
  })
}

@DaedalusDev
Copy link

DaedalusDev commented Jan 18, 2019

This is my solution :

/**
 * Check registred module
 * @param {Array} aPath - path to module - ex: ['my', 'nested', 'module']
 * @return {Boolean}
 */
Vuex.Store.prototype.hasModule = function (aPath) {
  let m = this._modules.root
  return aPath.every((p) => {
    m = m._children[p]
    return m
  })
}

Enjoy !

@ydennisy
Copy link

Is there a plan to include this in the core library?

@LeoniePhiline
Copy link

LeoniePhiline commented Dec 10, 2019

@KRaymundus
You'd need to move your let pathArray = path instanceof Array ? path : path.split('/'); from hasModule() into registerModuleOnce().

Because this.registerModule() expects an array for nested paths and does not do any splitting by '/' itself. This way, in your edited solution from 4 July 2019 any subsequent hasModule() will always fail to find the previously registered module (since it will have been registered as child of the root module by a name containing a slash).

Edit: It's probably easier explaining with code:

Vuex.Store.prototype.registerModuleOnce = function registerModuleOnce(path, rawModule, opts) {
  // Path can be an array or a string: ['my', 'nested', 'module'] or 'my/nested/module'
  const pathArray = path instanceof Array ? path : path.split('/');
  if (this.hasModule(pathArray)) {
    return false;
  }
  return this.registerModule(pathArray, rawModule, opts);
};

Vuex.Store.prototype.hasModule = function hasModule(pathArray) {
  let m = this._modules.root;
  return pathArray.every(p => {
    m = m._children[p];
    return m;
  });
};

@Paul-Cl-ark
Copy link

Paul-Cl-ark commented Feb 28, 2020

Any updates on this?

@kiaking
Copy link
Member

kiaking commented Mar 3, 2020

Not yet seems like. This is pretty old issue, though I don't think we came to any conclusion yet. @ktsn Do we want this feature...? Or should we be closing it?

@glutaminefree
Copy link

Hi, folks! I need this feature too.

@ktsn
Copy link
Member

ktsn commented Mar 25, 2020

#834 is merged.

@ktsn ktsn closed this as completed Mar 25, 2020
@Armaldio
Copy link

@ktsn When do you expect an npm release ?

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

No branches or pull requests