Extend plugin functionalities

[posva]

WIP

The idea of this PR is to give plugins more than just subscribing:

• Adding anything to the context passed to actions. This would allow constants or custom methods and can be useful to integrate a Database or Service like firebase.
• Adding new actions
• Adding new getters
• Adding new mutations

There's no need to add something to the state because it can be achieved by doing Vue.set(store.state, key, value). Although this should be pointed out in the docs

---

The content of this PR is still unfinished so we can iterate over the API before releasing it

---

API proposal

Adding properties to the context

Using functions allow to let the plugin check the path and use the context:

// multiple names are possible like assignContext, mergeContext, registerInContext, etc
mergeInContext((modulePath, context) => {
  // path is an array of strings: ['some', 'module']
  // Only registers for some/module
  if (modulePath.join('/') === 'some/module') {
    return {
      foo: 'staticProperty',
      bindRef () {
        context.commit(modulePath.join('/') + '/myPlugin/MUTATION')
      }
    }
  }
})

// registering in every context (most of the time)
mergeInContext((modulePath, context) => ({
  foo: 'staticProperty',
  bindRef () { // a function }
}))

This needs some kind of walk method on Modules that recursively invoke a
function. ModuleCollection's walk can simple be called on the root module

Then we can use the merged data in actions:

// merging properties directly into context
function someAction ({ commit, foo, bindRef }) {
  foo === 'staticProperty'
  bindRef()
}

// OR merging them in a plugins property
function someAction ({ commit, plugins: { foo, bindRef } }) {
  foo === 'staticProperty'
  bindRef()
}

// OR forcing a first parameter as the name of the plugin
// this name should not override existinting properties or at least
// display a warning
function someAction ({ commit, firebase: { foo, bindRef } }) {
  foo === 'staticProperty'
  bindRef()
}

Merging in a plugins property would prevent plugins to break with future
releases of Vuex (as pointed out by @ktsn) but also make it impossible for them
to overwrite the commit function or any other

Adding actions, mutations and getters

Differently from adding to context, when adding actions, mutations or getters,
you may want to add them to a specific module (imagine something like `plugins: [MyPlugin('moduleA', 'moduleB').
You may also want to add it to every module existing. This would be
the case for VuexFire, allowing it to use the context-specific commit functions
to allow the user to bind firebase references in any module.

// We should use the same name of context
mergeInModule((modulePath, context) => ({
  mutations: {
    // Dynamic names
    [`${modulePath.join('/')}/myPluginName/MUTATION`] (state) { // state.foo = 'bar' }
  }
}))

I'm not sure about what to do with the namespace. I think not using it may be
safer since plugins relying on it to register different mutations would fail

Extra things in this PR:

• Added a context to the rootStore (same as modules) accessible as store.context
• Moved plugin test to plugin.spec.js

[posva]

Hey @ktsn @yyx990803, what do you think of this API? (looking at the specs should be enough)
I also thought about registering in some custom or plugins property in the context, making impossible to override the commit and dispatch methods but also making the usage less convenient:

action ({ commit, plugins: { bindRef } }) {}

I also fear using common names like bind can make multiple vuex plugins incompatible (two plugins using the same name in context) but it should be enough to point it out in the docs

[ktsn]

Hmm, it's difficult problem... IMHO it would be better to directly register into context. I think it's enough to provide warning if the user registers something already exists.

There is one more concern on direct registration - there might be collisions between the name of injected thing by plugins and a possibly new API that Vuex has in the future. But it is a rare case, and if registerInContext always overwrites the existing property in context (with warnings), existing apps are still working. So I think it's not large problem.

BTW, it would make more sense to register in all contexts, wouldn't it? I'm not sure if we want to register in some specific module's contexts. If you intend to register different things for each module, what about letting to define generator function?

// In plugin
registerInContext('foo', (modulePath) => {
  return function registeredFn () {
    console.log(modulePath)
  }
})

// In module `bar`
actions: {
  someAction ({ foo }) {
    foo() // -> ['bar']
  }
}

[posva]

Thank @ktsn !

There is one more concern on direct registration - there might be collisions between the name of injected thing by plugins and a possibly new API that Vuex has in the future. But it is a rare case, and if registerInContext always overwrites the existing property in context (with warnings), existing apps are still working. So I think it's not large problem.

Also, a plugin can specify which version it is compatible with. If we allow overwriting properties in the context, I'm not sure a warning is a good idea.

BTW, it would make more sense to register in all contexts, wouldn't it? I'm not sure if we want to register in some specific module's contexts. If you intend to register different things for each module, what about letting to define generator function?

Absolutely. When defining in contexts, you probably want to define in every context. Also, I realised we cannot access dynamically registered modules. I'll even do something like:

// In plugin
registerInContext('foo', (path, context) => {
  return function registeredFn () {
    console.log(path)
    context.commit('somePluginMutation')
  }
})

I don't see a use case where you may not want to register in a specific context but if we ever need it to control that we can allow the function to return undefined or null to skip registration in the module

[ktsn]

If we allow overwriting properties in the context, I'm not sure a warning is a good idea.

My concern is that it might break a plugin if we disallow overwriting. For example, a plugin register a function foo in context, then Vuex introduce a same named api foo, finally the foo from the plugin is rejected even if it is a minor update.

I'll even do something like:

Looks great!

[posva]

@ktsn I reconsidered and I think we should disallow overwriting Vuex properties because I didn't find valid use cases for overwriting vuex inner api and it's leaner to do so since we can always reconsider it in the future while the opposite is not possible

[ktsn]

OK, makes sense 🙂

[jacoborus]

Hi guys, I am here looking for a way to expose an object to the actions through the context, so this is very interesting for me. Any plans to merge this commit?

[posva]

It's not ready yet. It'll be soon. Probably in 1 week :)

…

On Thu, 19 Jan 2017, 19:55 Jacobo Tabernero, ***@***.***> wrote: Hi guys, I am here looking for a way to expose an object to the actions through the context, so this is very interesting for me. Any plans to merge this commit? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#571 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAoicfJTJCAZCzsjB4IkMGv253z3rgdbks5rT7GogaJpZM4Ldecs> .

[jacoborus]

I just spent some time playing with the branch in my project, the functionality looks good for me, but I miss some syntax to make the use of registerInContext more handy in order to be able to register multiple properties to context, and also to register to more than one module at the same time. This is my proposal:

Register foo and bar in context of all modules and main store:

new Vuex.Store({
  modules: {
    moduleOne,
    moduleTwo
  },
  plugins: [(store, {registerInContext}) => {
    registerInContext({
      foo: 'foo',
      bar: 'bar'
    })
  }]
})

Register foo just in main store context (not in modules):

new Vuex.Store({
  modules: {
    moduleOne
  },
  plugins: [(store, {registerInContext}) => {
    registerInContext({ foo: 'foo' }, [])
  }]
})

Register foo just in context of moduleOne and main store:

new Vuex.Store({
  modules: {
    moduleOne,
    moduleTwo
  },
  plugins: [(store, {registerInContext}) => {
    registerInContext({ foo: 'foo' }, ['moduleOne'])
  }]
})

[posva]

@jacoborus I really think the callback is a more flexible approach to defining in which modules it should be registered. Furthermore, it allows to share the context property (commit, state, etc). However, I realised that having a merge syntax is more appropriate. This was similar to what @Akryum said about merging:

// In plugin
mergeInContext((path, context) => ({
  foo () {
    console.log(path)
    context.commit('somePluginMutation')
  }
}))

@ktsn WDYT?

[Akryum]

What I proposed was:

assignModule('some.module', {
  state: {
    newType: 'hello',
  },
  getters: {
    newGetter: state => state.newType,
  },
  mutations: {
    'some/namespace/NEW_MUTATION': (state, payload) => state.newType = payload,
  },
  actions: {
    'some/namespace/NEW_ACTION' ({ commit }) {
      commit('some/namespace/NEW_MUTATION', 'something')
    },
  },
})

And it could be complemented with:

assignContext('some.module', (path, context) => {
  return {
    ...context,
    config: { /* ... */ },
    foo () {
      console.log(path)
      context.commit('somePluginMutation')
    },
  }
})

// All the store
assignContext((path, context) => {
  return { /* ... */ }
})

[ktsn]

I'm not sure if merging is a good idea for the context. The merging syntax would encourage the users to register many things on the context but I think it should be avoided. For that case it would be appropriate to register an object that has several methods (i.e. namespacing)

registerInContext('pluginName', (path, context) => ({
  foo () {
    console.log(path)
    context.commit('somePluginMutation')
  },
  bar () {
    // ...
  },
  baz () {
    // ...
  }
}))

[posva]

@ktsn Why would that encourage users to register many things?
I also thought about namespacing, it feels less natural when using the plugin registered methods but it's great too:

someAction ({ commit, firebase: { bind, unbind }) {
}

someAction ({ commit, bindRef, unbindRef }) {
}

[ktsn]

Ah, I see. I forgot about (path, context) => { part. I thought it's ok to write registerInContext in 2~3 times. But considering to write the callback, merging is better.

[posva]

Yes, so we can reuse the logic if any :)

…

On Sat, 21 Jan 2017, 18:26 katashin, ***@***.***> wrote: Ah, I see. I forgot about (path, context) => { part. I thought it's ok to write registerInContext in 2~3 times. But considering to write the callback, merging is better. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#571 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAoicf_1efueU9YMGTBYwi2l8w3GzOhQks5rUj_fgaJpZM4Ldecs> .

[posva]

I updated the issue to reflect the discussed points. @ktsn can you take a look to make sure I didn't forget anything? 🙂

[ktsn]

@posva LGTM 👍

[jacoborus]

Hi @posva @ktsn, I need this feature for a development, do you believe it is too risky to start using @posva plugins branch? I don't care about having to change the syntax of my code later on, but I am worried about the feature not being implemented. Thanks in advance

[posva]

@jacoborus Yes, it is risky, you shouldn't do it. Don't worry, we'll look at this when the time has come 🙂

[jacoborus]

ok , thanks

[yyx990803]

@posva sorry for the late review.

1. First impression is that adding properties to action context seems to be separate from adding actions/mutations etc. to modules. I don't think it's a good idea to allow plugins to modify existing modules, unless there's a particularly convincing use case.

2. So, let's focus on the idea of injecting properties into the action context - this is essentially "action enhancing", but I am not entirely sure if it should be done at the plugin level. What I personally would do is directly enhancing the registered action function itself:

   actions: {
     someAction: enhanceWithFirebase(ref, ({ commit, bind, unbind }) => {
       // ...
     })
   }

   Also see an example in vuex-observable.

   Doing it at the action level also gives more flexibility / granular control, and avoids conflicting injections from multiple plugins. Better yet this doesn't introduce any additional complexity in Vuex itself.

[jacoborus]

@yyx990803 regarding to point 2, I found @posva solution very handy in order to avoid the load and injection of the same external instance in all the actions of all my modules; also the fact that registerInContext provides the context through the callback is good to allow that instance to use the Vuex store if needed

[posva]

@yyx990803 no worries 🙂

To 1: Yes, they're separated. My use is Vuexfire: I commit mutations to mutate an array bound to the state (https://github.com/posva/vuexfire/blob/v2/src/index.js#L64-L82). In order to let the user bind in any module, I commit mutations that mutate the state of the module where we currently are. So I'd like to add Mutations transparently in every module instead of having the user to merge an object of mutations generated by a function that needs the module name to be passed as an argument. The user will never commit those manually after all.

To 2: Yes, it's action enhancing that I'm trying to enhance 😛.

• It gets very annoying to do it everywhere, especially if you have to apply multiple enhancers on the same action
• The concept of enhancers deals with closures and is an advanced concept of js, making it harder to create plugins
• We cannot know which module we are in
• We can avoid injections of different plugins by providing a name when registering in context

Combining both ideas, it's easy to connect external data stores (could be a database). At the end the plugin usage would be extremely simple:

Add it as a plugin

const store = new Vuex.Store({
  // your options
  plugins: [VuexFire]
})

Use it

// actions.js
export function setTodosRef({ bind }, { ref }) {
  bind('todos', ref)
}
// if using plugin names:
export function setTodosRef({ firebase: { bind } }, { ref }) {
  bind('todos', ref)
}

---

This is a simplified version of the enhancer I had in mind for vuexfire, in case I'm missing something:

function firebaseAction (fn) {
  // Keep track of bound refs. Used in unbind
  const listeners = Object.create(null)
  const sources = Object.create(null)

  return function firebaseAction (context, payload) {
    // get access to commit, state and others as in the proposal
    const { state, commit } = context
    context.bind = function bind (key, source, cancelCb) {}
    context.unbind = function unbind (key) {}

    fn(context, payload)
  }
}

[yyx990803]

@posva the usage simplicity does make sense. I think we can potentially unify the two concepts internally by changing the plugin interface to something more like "applying a global action enhancer" (kinda like Redux middleware):

export const firebaseAction = actionFn => {
  return function enhancedActionFn (context, payload) {
    // ...
  }
}

export const plugin = store => {
  store.applyGlobalActionEnhancer(firebaseAction)
}

This way the firebaseAction can be used individually, or installed globally by the plugin.

[posva]

@yyx990803 mhh, I like the idea of global enhancers. However, the user still needs to register the mutations on each module and pass every time the module name, which is repeated on the store as well. I thought about registering a new module with the plugin but I don't see how it's possible to, later on, connect the state from that module to other modules. I thought about using getters but if someone binds a ref to the state of a module, he expects it to be there as well

[yyx990803]

Ah I see, I missed the connection between mutations there. Can you provide a more complete example of how you imagine VuexFire would be used? Do these mutations really need to happen in the module though?

[posva]

Sure, it's actually already implemented at https://github.com/posva/vuexfire/blob/v2/src/index.js
An example here: https://github.com/posva/vuexfire/blob/v2/examples/TodoApp/index.html

Ideally, I'd like to remove these parts in the example

// L52
const { bind, unbind } = VuexFire.generateBind(store)
// for a module: const { bind, unbind } = VuexFire.generateBind(store._modules.get(['todos']))
// L40
mutations: VuexFire.mutations,

It should happen the same way in modules and in the store. I need those mutations to happen on the module so the user can bind('myKey', ref) inside of an action in a module named module and then get state.module.myKey bound so he can use getters. He also needs to define the myKey property in the state of module upfront. Ping me on slack if something is unclear, I have notifications turned on

[JiriChara]

This is exactly what I need for a plugin I am working on. In my first attempt I have been naively thinking that this inside of actions, mutations and getters points to the store or to the current module and that one can simply attach something to the store and it would be available in the context, because that's the kind of way plugins for Vue are working.

const actions = {
  asyncInc(context) {
    // $something can be registered in the plugin
    this.$something(context).then(() => commit('inc'));
  }
};

I have then realized that this doesn't point to anything. Do you think it would make sense to call action, mutations and getters in context of the store? In Vue all methods, lifecycle methods, computed props are pointing to the current component instance as well.

[JiriChara]

Plugins can be defined as this:

const myPlugin = (store) => {
  // Define instance method that will be available in getters, mutations and actions
  store.$something = (args) => { /* ... */ }

  store.subscribe((mutation) => { /* ... */ });
};

What do you think @posva @ktsn @yyx990803?

[JiriChara]

https://github.com/JiriChara/vuex/commit/46afdb6fd59b72ae152732c23a3c8d12f4d2afb6

[posva]

@JiriChara That last thing is actually a bad idea because it'd make testing harder. As for getters, they're pure functions. Actions don't need access to the store. We'll probably go with actions enhancers, but other things in Vue core are more important at the moment

[JiriChara]

@posva OK, I agree on keeping all methods pure, but I must admit that I was little bit surprised when I realized that actions and mutations are not executed in context of the store. I have expected Vuex to behave same as Vue.

Global enhancers would be nice anyway.

[posva]

We're finally going in the way of action enhancers. A working version can be found at https://github.com/posva/vuexfire/blob/master/src/index.js#L182

It works exposing a mutation that receives the context in the payload, making it possible to call other internal mutations (see https://github.com/posva/vuexfire/blob/master/src/index.js#L20)
That mutations must be added to the root Store and make things work in strict mode, modules and root store 🙂

I'll surely create a page in the docs for this, not today, but soon 😄