championswimmer / vuex-module-decorators

TypeScript/ES7 Decorators to create Vuex modules declaratively
https://championswimmer.in/vuex-module-decorators/
MIT License
1.8k stars 170 forks source link

Cross-request state pollution ? #146

Closed arenddeboer closed 4 years ago

arenddeboer commented 5 years ago

I run into the classic cross-request state pollution scenario that is most often encountered when not using the stateFactory option. This is custom vue ssr project, modeled after the vue hackernews 2.0 repo.

I create my modules like this:

@Module({ stateFactory: true, namespaced: true, name: 'user' })
export class UserStoreModule extends VuexModule {
    public user: IUserModuleState['user'] = {}
}

This is my createStore function:

const createStore = () => {

    return new Vuex.Store({
        modules: {
            user: UserStoreModule
        }
    })
}
export default createStore

Module access is like this:

const userModule = getModule(UserStoreModule, this.$store)

I have setup a test environment to check if userModule.user.id is set just after initializing the store and module, and userModule.user.id is indeed set with the value from the previous request. When I modify the file dist/cjs/index.js and change: modOpt.store = modOpt.store || store; to modOpt.store = store || modOpt.store;

and

function getModule(moduleClass, store) {
    if (moduleClass._statics) {
        return moduleClass._statics;
    }

to

function getModule(moduleClass, store) {
    if (moduleClass._statics && !store) {
        return moduleClass._statics;
    } 

This issue goes away, it looks the previous state is stored and reused somewhere and forcing to use the store provided to getModule() helps prevent this issue.

Strangely enough, I can't reproduce this with a clean nuxt project and the nuxt way of setting it up as seen in the readme. Does nuxt do some magic by creating a new module, other than my user: UserStoreModule initializing ?

Sharsie commented 5 years ago

@arenddeboer I searched here and there and could not find a real solution anywhere... several hours later, I hoped to have a working solution for SSR, but as I dug deeper into the source code, I realized it's not as simple as previously thought.

So there are two main issues.

Caching issue

Whenever function getModule is called, it first looks up _statics property on the Module class. If not found, it generates a new one and sets it into _statics. _statics is basically your generated @Module class

This property includes stuff like state, store and other shenanigans...The problem is that we will share these "statics" when we receive a couple of requests at the same time... we could get rid of it or skip generating state and just generate mutations etc as these will not change...

Store being stored with the Model

Much bigger issue become the following lines of code from the module generator. It creates a _genStatic method on the Module Class (this is called when getModule is invoked) that accesses an outside property modOpt which again, is shared between requests during SSR. modOpt being @Module options. It also uses the store from modOpt by default, eventually overriding it if it is not defined in @Module ... what this effectively does is that it sets the store into modOpt object the first time you call getModule with store arg or when you register your @Module with store parameter (you either must set it in @Module or you must provide it in getModule, so one of these times it will get set)

So, we could switch the condition from modOpt.store || store to store || modOpt.store but guess what...modOpt is static and shared on SSR, so again, the users would be setting the store between each other with the last one winning the fight and providing their version for everyone.

I am not sure what Nuxt uses, I am using vue SSR without it, but this led me to the conclusion, that this repository cannot be used in SSR without a larger overhaul. Shame really, because I really liked this one... I'll try a couple more and then I will probably go back to untyped vuex because I will run out of time.

Addendum

Actually there could be a solution that might not be too difficult to implement, but is super hacky... It would require storing the statics into the store provided through getModule and rewriting all modOpt.store calls to use the statics from this store.

something along the lines of

function getModule(moduleClass, store) {
  var moduleName = '_vuexModuleDecorator_'+moduleClass.constructor.name;
  if (store[moduleName]) {
    // Oh yeah, javascript, can do anything
    return store[moduleName];
  }
  const statics = genStatic(store);

  // yay for recursion
  return (store[moduleName] = genStatic(store));
}
// Simplified version, genStatic is actually generated on the module class
function genStatic(store) {
  const statics = { store };

  staticStateGenerator(module, modOpt, statics);
  // Other staticaly generated shenanigans

  return statics;
}
function staticStateGenerator(module, modOpt, statics) {
// most of body removed for clarity
-                        return modOpt.store.state[modOpt.name][key];
+                        return statics.store.state[modOpt.name][key];
}
// Other static generators

I'll see if I can make a fork tomorrow and adjust it for SSR. Can't guarantee it will work 100% safe when hundreds of request come knocking on the door, but I believe it could work and only testing will show up any issues with this approach.

arenddeboer commented 5 years ago

Hi @Sharsie thanks for your extensive info. I have tested my "fix" using multiple concurrent sessions without problems. So I hope this will do for now. Would be interesting to see if your new solution works. But you are right, going back to plain old vuex is probably the safest bet considering the risks involved. I should probably dive a bit deeper into nuxt.js handling of modules as I would expect more people to have this issue if it is present in the vuex-module-decorators/nuxt.js combo.

Sharsie commented 5 years ago

Hey @arenddeboer I think you will run into problems once you start using Actions server side.

That's because the decorator factory for Actions is calling getModule without store... and that's where store will be used from the first generated _statics property for all users.

I'll post my workaround in a bit, I have ran into issues with this context in typescript and rollup.js, but I have come up with a solution that uses root store getters to save the generated class, no way around this without a complete overhaul of the repo, but it is a safe solution

In the meantime, here is a snippet that should produce state polution server side even with your fix, you can check yourself if that's the case for you as well:

https://gist.github.com/Sharsie/8a20a0d9dfe4e222dadae576cd198c58

It should produce the following output in the SSR logs:

First request

frontend_1     | === BEGIN ===
frontend_1     | Value of test using getModule state:
frontend_1     | initial
frontend_1     | Value of test directly from store
frontend_1     | initial
frontend_1     | Value of test using getModule state:
frontend_1     | Fri Jul 26 2019 10:13:07 GMT+0000 (Coordinated Universal Time)
frontend_1     | Value of test directly from store
frontend_1     | Fri Jul 26 2019 10:13:07 GMT+0000 (Coordinated Universal Time)

Second request

frontend_1     | === BEGIN ===
frontend_1     | Value of test using getModule state:
frontend_1     | Fri Jul 26 2019 10:13:07 GMT+0000 (Coordinated Universal Time)
frontend_1     | Store for the current request was polluted
frontend_1     | Value of test directly from store
frontend_1     | initial
frontend_1     | Value of test using getModule state:
frontend_1     | Fri Jul 26 2019 10:15:10 GMT+0000 (Coordinated Universal Time)
frontend_1     | Value of test directly from store
frontend_1     | initial
frontend_1     | Store for the current request was not changed

And in client logs:

First request

Note that I replace state client side with window.__INITIAL_STATE__ and therefor I have the state preset from SSR already

=== BEGIN ===
Value of test using getModule state:
Fri Jul 26 2019 10:13:07 GMT+0000 (Coordinated Universal Time)
Value of test directly from store
Fri Jul 26 2019 10:13:07 GMT+0000 (Coordinated Universal Time)
Value of test using getModule state:
Fri Jul 26 2019 12:13:08 GMT+0200 (Central European Summer Time)
Value of test directly from store
Fri Jul 26 2019 12:13:08 GMT+0200 (Central European Summer Time)

Second request

Note that __INITIAL_STATE__ now has a value of initial in the test property unlike during the first request. This is because server side the module is accessing the store from the first request, not modifying the store from the current request... therefor the test property is actually never changed for consecutive requests

=== BEGIN ===
Value of test using getModule state:
initial
Value of test directly from store
initial
Value of test using getModule state:
Fri Jul 26 2019 12:15:11 GMT+0200 (Central European Summer Time)
Value of test directly from store
Fri Jul 26 2019 12:15:11 GMT+0200 (Central European Summer Time)
Sharsie commented 5 years ago

Here is my working solution: https://github.com/championswimmer/vuex-module-decorators/compare/master...Sharsie:hotfix/ssrSupport Sharsie/vuex-module-decorators#hotfix.ssr.1

@championswimmer Is this something you would be willing to merge? Shall I PR the changes? I've done my best to preserve backward compatibility using _statics when store is not provided.

I can think of only one way this would break and that's when someone would mix usage of getModule(module) and getModule(module, store), then two instances of the module would be created... but then again, this might be a desired behaviour, e.g. using two different stores: getModule(module, store1) and getModule(module, store2)

The behaviour of this would change with the PR, as now it always uses 1 module state... after the PR each module would use state from a different store.

arenddeboer commented 5 years ago

Thanks @Sharsie. It passes my tests for this issue and I see no other problems after running a full suite of tests on my project. I'm going for your fork and hope this can be merged.

AliLozano commented 5 years ago

Can you make a pull request to this repo?. I have the same problem.

Sharsie commented 5 years ago

PR made, @AliLozano in the meantime, you can use npm i Sharsie/vuex-module-decorators#hotfix.ssr.1

AliLozano commented 5 years ago

Yes, I had already put it like that, I meant I didn't see the pull request to give +1.

Thanks.

Sharsie commented 5 years ago

I'll have to fix the tests in the PR, but I won't be able to look into it until next week, if someone wants to tackle it, feel free

nles commented 5 years ago

Anything new related to this one? Sharsie/vuex-module-decorators#hotfix.ssr.1 has been working nicely for us. I would love to see this merged to master, since vuex-module-decorators is pretty much unusable for us without SSR support...

Sharsie commented 5 years ago

@nles https://github.com/championswimmer/vuex-module-decorators/pull/157#issuecomment-521947047 Maybe if someone could help me resolve the rest of the CI issues, we could get this merged faster

codecov/patch, not really sure what tests it wants from me and how to write these...

Also codebeat is being a dick https://codebeat.co/projects/github-com-championswimmer-vuex-module-decorators-master/pull_requests/254560

It's because of this change: https://github.com/championswimmer/vuex-module-decorators/pull/157/commits/da68fd2ea70d1ff3afde9bf26d09bb76c916b67f#diff-c64086a10da3627b09c2725519e062d9L28-R31

I would have to move part of the code into its own function in order to resolve codebeat issue, but I would hate to refactor the code because of such a simple change, I think that's up to owners to refactor the rest of the code if they wish to do so :)

Sharsie commented 4 years ago

@arenddeboer The PR fixing this was merged, we can close the issue

arenddeboer commented 4 years ago

Fantastic