Feature: allow same paradigm for lazy loading modules #839

blake-newman · 2017-06-23T16:07:52Z

As described in #837

This will keep the same paradigm in Vuex as rest of ecosystem for dynamically loading store modules:

store.registerModule('a', () => import('@/store/modules/a'))

ktsn · 2017-06-24T17:39:35Z

test/unit/modules.spec.js

+      mutations: { foo: mutationSpy }
+    })
+
+    store.registerModule(['a', 'b'], module()).then(() => {


Shouldn't this be store.registerModule(['a', 'b'], module).then(() => { ?
(don't call the module function here)

Yes good spot, either way it will work

ktsn · 2017-06-24T17:41:03Z

src/store.js

+      return module
+    }
+
+    if (isPromise(rawModule)) return rawModule.then(moduleResolved)


I think we need to check whether rawModule is a function or not for proposed API rather than Promise.

ktsn · 2017-06-24T17:43:08Z

src/store.js

+    const moduleResolved = module => {
+      this._modules.register(path, module.default || module)
+      installModule(this, this.state, path, this._modules.get(path))
+        // reset store to update getters...


There seems to be redundant spaces before the comment 🙏

ktsn · 2017-06-24T17:44:11Z

docs/en/api.md

@@ -156,10 +156,10 @@ const store = new Vuex.Store({ ...options })

  Most commonly used in plugins. [Details](plugins.md)

- **`registerModule(path: string | Array<string>, module: Module)`**
+- **`registerModule(path: string | Array<string>, module: Module|Function): Module|Function`**


Might be better to add a space around | ?

ktsn · 2017-06-25T20:15:23Z

src/store.js

+      installModule(this, this.state, path, this._modules.get(path))
+        // reset store to update getters...
+      resetStoreVM(this, this.state)
+      return module


Why do we need to return the module here? I have no idea about use cases 🤔

ktsn · 2017-06-25T20:15:57Z

src/store.js

+    }
+
+    if (isPromise(rawModule)) return rawModule.then(moduleResolved)
+    return moduleResolved(rawModule)


It would be better to always return Promise for consistency?

If you return a Promise, it will mean you will need to ensure that Vuex is used with Promises which may require pollyfils. This will ensure users don't need it if they are following standard flow

Vuex requires Promise to work. So we can consider that Promise is always there 🙂

ktsn · 2017-06-25T20:44:39Z

I have a small concern of this API. When some getters/actions/mutations are used before loading the corresponding module, it can be failed.
I guess the returning Promise of registerModule is there for handling such situation. But I wonder if this syntax is needed for Vuex since we need to handle the loading completions of async modules in the user land. Or are you considering like lazy dispatch/commit to handle it in Vuex?

blake-newman · 2017-06-25T21:33:43Z

@ktsn I will run through the code and check some of points, some points may be right to change need to explore.

The main purpose of the API is to keep code splitting of modules simple to use and consistent with other parts of the Vue ecosystem.

Perhaps extra documentation for lazy loading is needed to make this feature more informative. I think having a section on Lazy loading dynamic modules would be great.

There is some gotchas users should be aware of, using the client asyncData mixins from SSR docs makes this easier. I was thinking that the client mixins are actually more fitting to be in main Vue docs as it can be used without SSR.

None the less we should highlight prerequisites and patterns required for this use, in the right environment this feature will be extremely powerful for applications that are large.

blake-newman · 2017-06-25T21:41:25Z

Furthermore doing the latter of your comment would invoke technical complexity, as this mains if you use an action to get changes is will be deferred.

The current API just makes it easier to use code splitting but will require user to correctly manage registering modules.

ktsn · 2017-06-25T21:45:25Z

@blake-newman
I see the point. I agree that the extra documentation for lazy loading would be informative 🙂

nickmessing · 2017-06-25T23:15:40Z

@blake-newman, thank you for the effort, was thinking about this feature for some time, would help me a lot.

ktsn · 2017-06-27T07:30:20Z

types/index.d.ts

@@ -22,6 +22,7 @@ export declare class Store<S> {

  registerModule<T>(path: string, module: Module<T, S>): void;
  registerModule<T>(path: string[], module: Module<T, S>): void;
+  registerModule<T>(path: string[], module: () => Promise<Module<T, S>>): Promise<Module<T, S>>;


Looks like it returns Promise<void>

ktsn · 2017-06-27T07:31:03Z

docs/en/modules.md

@@ -242,7 +242,57 @@ The module's state will be exposed as `store.state.myModule` and `store.state.ne

 Dynamic module registration makes it possible for other Vue plugins to also leverage Vuex for state management by attaching a module to the application's store. For example, the [`vuex-router-sync`](https://github.com/vuejs/vuex-router-sync) library integrates vue-router with vuex by managing the application's route state in a dynamically attached module.

-You can also remove a dynamically registered module with `store.unregisterModule(moduleName)`. Note you cannot remove static modules (declared at store creation) with this method.


Why did you remove this? 🤔

Looks to be an accident... Will ammend

ktsn

👍

yyx990803 · 2017-07-18T17:23:18Z

I like the idea but I'm not sure if this is entirely necessary, because reigsterModule is always used imperatively, you can already do:

asyncData (store) {
  return import('./foo.module.js')
    .then(m => store.registerModule('foo', m))
    .then(() => store.dispatch('foo/bar'))
}

Or

async asyncData (store) {
  store.registerModule('foo', await import('./foo.module.js'))
  await store.dispatch('foo/bar')
}

The primary reason for the async component syntax is because components need to be used in declarative scenarios where await or Promise resolving are not available. If we were to support async vuex modules with the same syntax, it makes more sense to support it when declaring stores:

const store = new Vuex.Store({
  modules: {
    foo: import('./foo.module.js')
  }
})

But this is extremely tricky to get right, since the state access is synchronous.

blake-newman · 2017-07-19T00:20:15Z

@yyx990803 agree. Probably just needs documentation rather than internal changes. IMO in hindsight this doesn't add much value other than syntax sugar that only abstracts implementation detail that is truly not that much more difficult.

alexjoverm · 2017-07-24T10:03:04Z

I agree on this is not necessary, but I consider that if it doesn't cause any trouble, it's a nice syntax sugar added.

When writing Lazy load in Vue using Webpack's code splitting, I could easily realize it's easy by doing it in the way Evan mentioned. But some people were expecting it to work in the same way than components or router (even some though registerModule understand import function already).

Aside: does it make sense to lazy load modules on declaration? I think the use case is to load them under a determined condition

blake-newman requested review from yyx990803 and ktsn June 23, 2017 16:07

blake-newman added the proposal label Jun 23, 2017

ktsn reviewed Jun 25, 2017

View reviewed changes

blake-newman force-pushed the feature/simplify-dynamic-registration branch 2 times, most recently from ff327e2 to 679a32e Compare June 26, 2017 16:43

ktsn reviewed Jun 27, 2017

View reviewed changes

blake-newman force-pushed the feature/simplify-dynamic-registration branch 3 times, most recently from 3ff2951 to 6bb72f7 Compare June 27, 2017 17:37

Feature: allow same paradigm for lazy loading modules

cfb3c65

blake-newman force-pushed the feature/simplify-dynamic-registration branch from 6bb72f7 to cfb3c65 Compare June 27, 2017 17:39

ktsn approved these changes Jun 28, 2017

View reviewed changes

nickmessing mentioned this pull request Jul 4, 2017

Provide first class support for splitting code base into modules vuejs/vue#6026

Closed

blake-newman closed this Jul 19, 2017

kiaking deleted the feature/simplify-dynamic-registration branch April 23, 2020 04:25

		@@ -242,7 +242,57 @@ The module's state will be exposed as `store.state.myModule` and `store.state.ne

		Dynamic module registration makes it possible for other Vue plugins to also leverage Vuex for state management by attaching a module to the application's store. For example, the [`vuex-router-sync`](https://github.com/vuejs/vuex-router-sync) library integrates vue-router with vuex by managing the application's route state in a dynamically attached module.

		You can also remove a dynamically registered module with `store.unregisterModule(moduleName)`. Note you cannot remove static modules (declared at store creation) with this method.

Uh oh!

Feature: allow same paradigm for lazy loading modules #839

Feature: allow same paradigm for lazy loading modules #839

Uh oh!

Conversation

blake-newman commented Jun 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktsn commented Jun 25, 2017

Uh oh!

blake-newman commented Jun 25, 2017

Uh oh!

blake-newman commented Jun 25, 2017

Uh oh!

ktsn commented Jun 25, 2017

Uh oh!

nickmessing commented Jun 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktsn left a comment

Choose a reason for hiding this comment

Uh oh!

yyx990803 commented Jul 18, 2017

Uh oh!

blake-newman commented Jul 19, 2017

Uh oh!

alexjoverm commented Jul 24, 2017

Uh oh!

Uh oh!