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

store logic is not reactive in data prefetching steps while doing ssr #877

Closed
Harper04 opened this issue Jul 21, 2017 · 13 comments
Closed
Labels
enhancement New feature or request

Comments

@Harper04
Copy link

Version

2.3.0

Reproduction link

https://github.com/Harper04/vuex/tree/bug/ssr

Steps to reproduce

Either:

Clone the linked repo and run npm i && npm run test:ssr

Or:

Run tests in your own repo with (this mimics the environment vue sees during ssr rendering):
VUE_ENV=server npm run test:unit

Or:

Have a project with a lot of stores and watch for weird ssr behavior

What is expected?

The tests should run.
Your store logic should behave exactly the same no matter whether you run it in your browser or during an ssr data prefetching step.

What is actually happening?

Vue turns of reactivity in case of server side rendering the moment you create/import the an vue ssr renderer. For frontend components/real rendering this is perfectly okay and an important optimization.
The problem is that vuex also uses a vue instance internally in order to get the reactivity and caching for getters etc. Therefore also the reactivity of vuex is turned of during ssr. Every getter will return whatever it returned the first time it was called (think of isValid getters or filtered lists)


Debugging:

I did run the tests and my project with a modified version of vue where the isServerRendering check in the observer part of Vue is deactivated which worked fine.
I also tried to just return the getter function instead of the computed property in mappedGetters (vuex) which also worked for me (but not for the tests).

PR?

I've tried to come up with a pull request where vue would get another constructor variable like forceReactivity or dataOnlyVueInstance which vuex could pass in during instantiation.
Unfortunately apart from hacks i could only think of solutions which meant to much work for the risk of solving it in a way you would not accept.
If you could show me a way to pass vm.$options parameters to the observer module without touching too much code i would still be happy to do a PR.

Am i right here?

This seems like a bug for vuex which should be solved by enhancing vue itself. Should i create a feature request there?

@LinusBorg
Copy link
Member

Hi, and thanks for this!

Can you point me to the test(s) that are failing as I can't run them currently? So I can take a look to better understand the implicatons right now?

@Harper04
Copy link
Author

Here i pasted the output for you (github does not support collapsing of blocks, right?...)

VUE_ENV=server npm run test:unit

> vuex@2.3.1 test:unit /Users/anonuser/Desktop/Code/gits/vuex
> rollup -c build/rollup.dev.config.js && jasmine JASMINE_CONFIG_PATH=test/unit/jasmine.json

Started
FFF....FFF...F..F...........F..........................F..FF......

Failures:
1) Helpers mapState (array)
  Message:
    Expected 1 to be 2.
  Stack:
    Error: Expected 1 to be 2.
        at Object.<anonymous> (/Users/anonuser/Desktop/Code/gits/vuex/test/unit/helpers.spec.js:17:18)

2) Helpers mapState (object)
  Message:
    Expected 3 to be 4.
  Stack:
    Error: Expected 3 to be 4.
        at Object.<anonymous> (/Users/anonuser/Desktop/Code/gits/vuex/test/unit/helpers.spec.js:39:18)

3) Helpers mapState (with namespace)
  Message:
    Expected 3 to be 5.
  Stack:
    Error: Expected 3 to be 5.
        at Object.<anonymous> (/Users/anonuser/Desktop/Code/gits/vuex/test/unit/helpers.spec.js:64:18)
  Message:
    Expected 3 to be 7.
  Stack:
    Error: Expected 3 to be 7.
        at Object.<anonymous> (/Users/anonuser/Desktop/Code/gits/vuex/test/unit/helpers.spec.js:68:18)

4) Helpers mapGetters (array)
  Message:
    Expected false to be true.
  Stack:
    Error: Expected false to be true.
        at Object.<anonymous> (/Users/anonuser/Desktop/Code/gits/vuex/test/unit/helpers.spec.js:181:23)
  Message:
    Expected false to be true.
  Stack:
    Error: Expected false to be true.
        at Object.<anonymous> (/Users/anonuser/Desktop/Code/gits/vuex/test/unit/helpers.spec.js:186:25)

5) Helpers mapGetters (object)
  Message:
    Expected false to be true.
  Stack:
    Error: Expected false to be true.
        at Object.<anonymous> (/Users/anonuser/Desktop/Code/gits/vuex/test/unit/helpers.spec.js:211:18)
  Message:
    Expected false to be true.
  Stack:
    Error: Expected false to be true.
        at Object.<anonymous> (/Users/anonuser/Desktop/Code/gits/vuex/test/unit/helpers.spec.js:216:18)

6) Helpers mapGetters (with namespace)
  Message:
    Expected false to be true.
  Stack:
    Error: Expected false to be true.
        at Object.<anonymous> (/Users/anonuser/Desktop/Code/gits/vuex/test/unit/helpers.spec.js:246:18)
  Message:
    Expected false to be true.
  Stack:
    Error: Expected false to be true.
        at Object.<anonymous> (/Users/anonuser/Desktop/Code/gits/vuex/test/unit/helpers.spec.js:251:18)

7) Helpers createNamespacedHelpers
  Message:
    Expected 0 to be 1.
  Stack:
    Error: Expected 0 to be 1.
        at Object.<anonymous> (/Users/anonuser/Desktop/Code/gits/vuex/test/unit/helpers.spec.js:367:22)
  Message:
    Expected true to be false.
  Stack:
    Error: Expected true to be false.
        at Object.<anonymous> (/Users/anonuser/Desktop/Code/gits/vuex/test/unit/helpers.spec.js:368:23)
  Message:
    Expected true to be false.
  Stack:
    Error: Expected true to be false.
        at Object.<anonymous> (/Users/anonuser/Desktop/Code/gits/vuex/test/unit/helpers.spec.js:374:41)

8) Hot Reload getters
  Message:
    Expected 0 to be 1.
  Stack:
    Error: Expected 0 to be 1.
        at Object.<anonymous> (/Users/anonuser/Desktop/Code/gits/vuex/test/unit/hot-reload.spec.js:268:18)
  Message:
    Expected 0 to be 1.
  Stack:
    Error: Expected 0 to be 1.
        at Store.check (/Users/anonuser/Desktop/Code/gits/vuex/test/unit/hot-reload.spec.js:248:33)
        at Array.wrappedActionHandler (/Users/anonuser/Desktop/Code/gits/vuex/dist/vuex.common.js:660:23)
        at Store.dispatch (/Users/anonuser/Desktop/Code/gits/vuex/dist/vuex.common.js:392:7)
        at Store.boundDispatch [as dispatch] (/Users/anonuser/Desktop/Code/gits/vuex/dist/vuex.common.js:308:21)
  Message:
    Expected 0 to be 10.
  Stack:
    Error: Expected 0 to be 10.
        at Object.<anonymous> (/Users/anonuser/Desktop/Code/gits/vuex/test/unit/hot-reload.spec.js:278:18)
  Message:
    Expected spy unknown to have been called.
  Stack:
    Error: Expected spy unknown to have been called.
        at /Users/anonuser/Desktop/Code/gits/vuex/test/unit/hot-reload.spec.js:282:19
        at Array.<anonymous> (/Users/anonuser/Desktop/Code/gits/vuex/node_modules/vue/dist/vue.common.js:703:14)
        at nextTickHandler (/Users/anonuser/Desktop/Code/gits/vuex/node_modules/vue/dist/vue.common.js:650:16)
        at process._tickCallback (internal/process/next_tick.js:109:7)

9) Modules module registration dynamic module registration
  Message:
    Expected 1 to be 2.
  Stack:
    Error: Expected 1 to be 2.
        at Object.<anonymous> (/Users/anonuser/Desktop/Code/gits/vuex/test/unit/modules.spec.js:40:31)
  Message:
    Expected 1 to be 2.
  Stack:
    Error: Expected 1 to be 2.
        at Object.<anonymous> (/Users/anonuser/Desktop/Code/gits/vuex/test/unit/modules.spec.js:42:33)

10) Store getters
  Message:
    Expected 'none' to be 'hasAny'.
  Stack:
    Error: Expected 'none' to be 'hasAny'.
        at Object.<anonymous> (/Users/anonuser/Desktop/Code/gits/vuex/test/unit/store.spec.js:237:33)
  Message:
    Expected 'none' to be 'hasAny'.
  Stack:
    Error: Expected 'none' to be 'hasAny'.
        at Store.check (/Users/anonuser/Desktop/Code/gits/vuex/test/unit/store.spec.js:228:33)
        at Array.wrappedActionHandler (/Users/anonuser/Desktop/Code/gits/vuex/dist/vuex.common.js:660:23)
        at Store.dispatch (/Users/anonuser/Desktop/Code/gits/vuex/dist/vuex.common.js:392:7)
        at Store.boundDispatch [as dispatch] (/Users/anonuser/Desktop/Code/gits/vuex/dist/vuex.common.js:308:21)

11) Store strict mode: warn mutations outside of handlers
  Message:
    Expected function to throw an exception.
  Stack:
    Error: Expected function to throw an exception.
        at Object.<anonymous> (/Users/anonuser/Desktop/Code/gits/vuex/test/unit/store.spec.js:274:39)

12) Store watch: with resetting vm
  Message:
    Expected spy unknown to have been called.
  Stack:
    Error: Expected spy unknown to have been called.
        at /Users/anonuser/Desktop/Code/gits/vuex/test/unit/store.spec.js:299:21
        at Array.<anonymous> (/Users/anonuser/Desktop/Code/gits/vuex/node_modules/vue/dist/vue.common.js:703:14)
        at nextTickHandler (/Users/anonuser/Desktop/Code/gits/vuex/node_modules/vue/dist/vue.common.js:650:16)
        at runMicrotasksCallback (internal/process/next_tick.js:64:5)

66 specs, 12 failures
Finished in 0.173 seconds

@Harper04
Copy link
Author

A simple example for you about the implications:
Let say you have a website with a store selling sandwiches (really just an example, i do totally different things).
You get a request like this: http://sandwiches.com/sandwiches/?sandwich=awseomsandwich

And have a store (pseudocode) like this:

state: {
  sandwiches: [],
  selectedSandwich: ''
},
getters: {
   availableSandwiches () { return filter(getters.allSandwiches)},
   allSandwiches() {return state.sandwiches}
},
mutations: {
   selectSandwich () { state.selectedSandwich = sandwich},
   importSandwiches () { state.sandwiches = sandwiches}
},
actions: {
  loadSandwiches(preselectSandwich) {
      if (getters.allSandwiches.length) {
          sandwiches = await fetchSandwiches()
          mutations.importSandwiches(sandwiches)
      }
      if (preselectSandwich in getters.availableSandwiches) {
         selectSandwich(preselectSandwich) 
      }
  }
}

loadSandwiches will load the Sandwich list and save it correctly to the store. Hower the preselection of awesomesandwich will fail because getters.allSandwiches will still return an empty list as the vue instance of vuex is not reactive in SSR mode.
So availableSandwiches will return an empty list causing the safety check to only select existing sandwiches will fail.

Vues SSR function will render a webpage with the sandwiches but without the preselection. The initial render of vue during $mount will match it because the sandwich ist not preselected in the store. Depending on your other data prefetching logic the website may flicker and select the sandwich afterwards because the browser tries to refetch the sandwiches (notices it already has some and skip that) but will preselect the Sandwich because now the getters are reactive.

In most cases the bug does not lead to crashes (had some but got workarounds working) because the initial rendering matches. But afterwards you may get a crappy user experience.

@LinusBorg
Copy link
Member

LinusBorg commented Jul 21, 2017

I think the best solution would be to create the computed properties of the getters with option { lazy: false } to deactivate the caching mechanism during SSR.

Here: https://github.com/vuejs/vuex/blob/dev/src/store.js#L216

something like:

computed[key] = () => ({
  handler() { fn(store) },
  lazy: !(process.env.VUE_ENV === 'server')
})

Maybe add an option to enable / disable this during new Vuex.Store(), because obviously disabling the caching behaviour would mean each time a getter is accessed, it's re-revaluated which means your app will render a bit slower

@LinusBorg LinusBorg added the enhancement New feature or request label Jul 21, 2017
@LinusBorg LinusBorg self-assigned this Jul 25, 2017
@LinusBorg
Copy link
Member

Will see if I can come up with a PR tonight.

@LinusBorg
Copy link
Member

Hey, so I was looking into this - the problem is that computed props are hardcoded to be lazy in Vue:

https://github.com/vuejs/vue/blob/dev/src/core/instance/state.js#L167

@yyx990803 : If we intend to fix this vuex problem by making getters non-lazy during SSR, this means we have to change something in Vue first - right?

@LinusBorg LinusBorg removed their assignment Aug 16, 2017
@yyx990803
Copy link
Member

I've incorporated the SSR env tests in 9438bab - note these tests will currently fail until Vue core's next patch release.

yyx990803 added a commit to vuejs/vue that referenced this issue Sep 1, 2017
@Harper04
Copy link
Author

Harper04 commented Sep 2, 2017

thanks! this will mean a lot for our user experience

@amritk
Copy link

amritk commented Oct 5, 2017

Is there a workaround to get this working?

@yyx990803
Copy link
Member

@amritk this is already out in Vue 2.4.4.

@amritk
Copy link

amritk commented Oct 6, 2017

@yyx990803 that is very strange, I'm still seeing the error on Vue 2.4.4, Vuex 2.4.1, and Vue-server-renderer 2.4.4
The following code gives this error:

[Vue warn]: The client-side rendered virtual DOM tree is not matching server-rendered content. This is likely caused by incorrect HTML markup, for example nesting block-level elements inside <p>, or missing <tbody>. Bailing hydration and performing full client-side render.
<template>
    <div>
        <span v-if="options">Hello</span>
    </div>
</template>

<script>
import { mapGetters } from 'vuex'

export default {
    computed: {
        ...mapGetters({
            options: 'toolbar/options'
        })
    }
}
</script>

Whereas this code works fine

<template>
    <div>
        <span>{{options}}</span>
    </div>
</template>

<script>
import { mapGetters } from 'vuex'

export default {
    computed: {
        ...mapGetters({
            options: 'toolbar/options'
        })
    }
}
</script>

@yyx990803
Copy link
Member

@amritk then you should open a separate issue with a proper reproduction

@amritk
Copy link

amritk commented Oct 6, 2017

Ah kk, sorry thought it was the same issue

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

No branches or pull requests

4 participants