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

Hotfix/ssr support #157

Merged
merged 9 commits into from Nov 3, 2019

Conversation

@Sharsie
Copy link
Contributor

Sharsie commented Jul 29, 2019

SSR breaks the modules by always using the Module version generated from the first request that reaches the server and creates the Module... then the vuex store used during this request is also shared between all consecutive requests.

This is because the generated instance of the @Module class is stored in its static property _statics and cached for later use (later use being also later requests made to the server)

This changes the behaviour so that the store provided to getModule() is used to save the generated instace of the @Module... store being recreated in SSR on each request ensures that the module is also recreated on each request.

More info in #146

Arnie added 3 commits Jul 26, 2019
If the store Module decorator uses dynamic: true, the store option
is also mandatory
When the @module decorator is used with Server Side Rendering it shared
the _statics property generated on the Module class between requests.

This adjustment stores the generated class into the root store which is
recreated every time a new request is made and therefor does not
pollute the static namespace server side
@codecov

This comment has been minimized.

Copy link

codecov bot commented Jul 29, 2019

Codecov Report

Merging #157 into master will decrease coverage by 3.22%.
The diff coverage is 39.13%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #157      +/-   ##
==========================================
- Coverage   82.92%   79.69%   -3.23%     
==========================================
  Files           9        9              
  Lines         123      133      +10     
  Branches       14       16       +2     
==========================================
+ Hits          102      106       +4     
- Misses         18       23       +5     
- Partials        3        4       +1
Impacted Files Coverage Δ
src/vuexmodule.ts 52.94% <14.28%> (-19.79%) ⬇️
src/module/staticGenerators.ts 47.36% <28.57%> (ø) ⬆️
src/helpers.ts 60% <50%> (-6.67%) ⬇️
src/module/index.ts 93.33% <66.66%> (+3.33%) ⬆️
src/action.ts 95.83% <75%> (-4.17%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 70965e9...a7fd2d0. Read the comment docs.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Jul 29, 2019

Codecov Report

Merging #157 into master will increase coverage by 12.52%.
The diff coverage is 96.15%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #157       +/-   ##
===========================================
+ Coverage   83.06%   95.58%   +12.52%     
===========================================
  Files           9        9               
  Lines         124      136       +12     
  Branches       14       16        +2     
===========================================
+ Hits          103      130       +27     
+ Misses         18        4       -14     
+ Partials        3        2        -1
Impacted Files Coverage Δ
src/helpers.ts 80% <100%> (+13.33%) ⬆️
src/vuexmodule.ts 94.11% <100%> (+21.39%) ⬆️
src/module/staticGenerators.ts 100% <100%> (+52.63%) ⬆️
src/module/index.ts 100% <100%> (+10%) ⬆️
src/action.ts 95.83% <75%> (-4.17%) ⬇️
src/module/stateFactory.ts 100% <0%> (+12.5%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da8ec22...77c1b3c. Read the comment docs.

@championswimmer

This comment has been minimized.

Copy link
Owner

championswimmer commented Aug 10, 2019

Could you add tests for the same ?

@Sharsie

This comment has been minimized.

Copy link
Contributor Author

Sharsie commented Aug 12, 2019

Sure, I'll try to get to it this week

@Sharsie

This comment has been minimized.

Copy link
Contributor Author

Sharsie commented Aug 16, 2019

Hey @championswimmer , I have a question, I was writing a test where you have 2 different stores use the module in each separately and by a chance I forgot to pass the statefactory: true option to the decorator.
Updating one module updated state of the same module in different store, which is something I would not expect by default.

And I am wondering, in our context where we are generating the module through getModule, you have two options:

  1. Not to use store and module will be generated in _statics property
  2. Use store and module will be generated on the provided store

From my point of view, in both cases, it does not make sense not to use the statefactory function.
In the fist case, you always get your single instance of the module... no matter if you have registered it on multiple stores or not.
In the second case, you would expect that module from store 1 would not share state with module from store 2

So making the statefactory function the default option would be backwards compatible and would actually make the option irrelevant, is there any reason why this option shouldn't be dropped in the context of this repo?

Arnie
getModule is tested when no name is provided, when module is fetched
from the same store multiple times and when module is fetched
from two different stores
@Sharsie Sharsie force-pushed the Sharsie:hotfix/ssrSupport branch from 64e3c82 to aa62603 Aug 16, 2019
Arnie added 3 commits Aug 16, 2019
Arnie
The condition was removed by a mistake in previous commits
Arnie
@Sharsie Sharsie force-pushed the Sharsie:hotfix/ssrSupport branch from 340b76c to ea445ac Aug 16, 2019
@Sharsie

This comment has been minimized.

Copy link
Contributor Author

Sharsie commented Aug 16, 2019

@championswimmer
I have added the tests, but codebeat is really giving me headaches... let me know what to do here, I don't want to refactor the code because of adding a single assignment in order to satisfy that tool

Not really sure what to do about the patch coverage

@danielroe

This comment has been minimized.

Copy link
Contributor

danielroe commented Sep 3, 2019

It would be really good to get this merged in @championswimmer

@iliyaZelenko

This comment has been minimized.

Copy link
Contributor

iliyaZelenko commented Sep 11, 2019

I have the same problem with Nuxt already 2 months ago, I had to spend many hours to find a solution. I reviewed all the use cases provided in the discussions of this package and nothing helped. As a result, I had to write my own hack, in which it is not convenient to write code.

This code will help everyone who has Nuxt/SSR and saves time for many people!

@championswimmer really looking forward to when it will be merged. Please see how time will be, thanks!

@Sharsie

This comment has been minimized.

Copy link
Contributor Author

Sharsie commented Sep 12, 2019

With some help from my colleague I understood how to cover the patch changes (although they were not tested before either), so fingers crossed for CI in a minute! :)

@championswimmer

This comment has been minimized.

Copy link
Owner

championswimmer commented Sep 13, 2019

looks good

@danielroe

This comment has been minimized.

Copy link
Contributor

danielroe commented Sep 22, 2019

Can we merge this @Sharsie?

@Sharsie

This comment has been minimized.

Copy link
Contributor Author

Sharsie commented Sep 22, 2019

@danielroe well sure, but I can't :)

@danielroe

This comment has been minimized.

Copy link
Contributor

danielroe commented Sep 22, 2019

@Sharsie Ah - I see, @championswimmer has approved it, but codebeat is ... objecting.

@danielroe

This comment has been minimized.

Copy link
Contributor

danielroe commented Oct 1, 2019

@championswimmer Can this be merged or does it need any further work?

@andrewvasilchuk

This comment has been minimized.

Copy link

andrewvasilchuk commented Nov 2, 2019

Please, merge this Pull Request :)

@danielroe

This comment has been minimized.

Copy link
Contributor

danielroe commented Nov 2, 2019

This is such a significant security hole for use with Nuxt that I've had to abandon using vuex-module-decorators entirely. Until this PR is merged, I'd recommend using @Sharsie's fork, switching to vuex-class-component (although that repo has gone silent), or reverting to vanilla Vuex.

See here for an overview of the options.

@championswimmer championswimmer merged commit 834b1d5 into championswimmer:master Nov 3, 2019
4 of 5 checks passed
4 of 5 checks passed
codebeat 0 issues resolved and 1 introduced
Details
LGTM analysis: JavaScript No new or fixed alerts
Details
codecov/patch 96.15% of diff hit (target 83.06%)
Details
codecov/project 95.58% (+12.52%) compared to da8ec22
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.