Fix hot reloading issues by removing the store from window #324
+45
−28
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DanielOrtel
commented
Feb 11, 2021
| } | ||
|
|
||
| const initStore = <S extends Store>({makeStore, context, config}: InitStoreOptions<S>): S => { | ||
| const storeKey = getStoreKey(config); | ||
| let store: any; |
There was a problem hiding this comment.
this may not even be necessary, but it's probably a good idea to keep a global reference around in case getInitialProps is called client side. Far as I could test, client side getInitialProps used the existing store and state
DanielOrtel
commented
Feb 11, 2021
| @@ -214,7 +214,6 @@ The `createWrapper` function accepts `makeStore` as its first argument. The `mak | |||
|
|
|||
| `createWrapper` also optionally accepts a config object as a second parameter: | |||
|
|
|||
| - `storeKey` (optional, string) : the key used on `window` to persist the store on the client | |||
There was a problem hiding this comment.
this is no longer necessary
DanielOrtel
force-pushed
the
fix-hot-reloading
branch
from
4c97e10
to
3063db7
Compare
DanielOrtel
commented
Feb 11, 2021
packages/wrapper/src/index.tsx
Outdated
| @@ -118,7 +115,7 @@ export const createWrapper = <S extends Store>(makeStore: MakeStore<S>, config: | |||
| callback: GetServerSidePropsCallback<S, P>, | |||
| ): GetServerSideProps<P> => async context => await getStaticProps(callback as any)(context); // just not to repeat myself | |||
|
|
|||
| const withRedux = (Component: NextComponentType | App | any) => { | |||
| const withRedux: (Component: NextComponentType | App | any) => NextComponentType | App | any = Component => { | |||
There was a problem hiding this comment.
type inference couldn't properly figure it out.
There was a problem hiding this comment.
nevermind, ended up reverting the change. Explicit typing is too much of a headache here, since it could be either Page or App, so a consumer would always need to specify which.
DanielOrtel
force-pushed
the
fix-hot-reloading
branch
from
3063db7
to
60156d4
Compare
DanielOrtel
commented
Feb 11, 2021
| const wrapper = createWrapper(makeStore, {storeKey: 'testStoreKey'}); | ||
| const Page = () => null; | ||
| Page.getInitialProps = wrapper.getInitialPageProps(store => () => null); | ||
| test('store is available when calling getInitialProps client-side and references the existing store on client', async () => { |
There was a problem hiding this comment.
I'm thinking this is sufficient to test the store is the same on client-side getInitialProps
DanielOrtel
force-pushed
the
fix-hot-reloading
branch
2 times, most recently
from
e516388
to
63f8648
Compare
|
Great job. Thank you. I will review and merge shortly. |
DanielOrtel
force-pushed
the
fix-hot-reloading
branch
from
63f8648
to
4618f1e
Compare
kirill-konshin
added a commit
that referenced
this pull request
Apr 17, 2021
* Fix #280 * Fix #207 * Minor fixes * Updated API * Updated versions * Update README.md * Fixed demos & tests * Github Action * Make demo-dynamic private * Fix #326 * Fix hot reloading issues by removing the store from window (#324) Co-authored-by: Melanie Seltzer <melleh11@gmail.com> Co-authored-by: Daniel Ferenc Balogh <danielferencortel@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #317