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

Hot reload is broken for reducers only. #317

Closed
DanielOrtel opened this issue Jan 10, 2021 · 8 comments
Closed

Hot reload is broken for reducers only. #317

DanielOrtel opened this issue Jan 10, 2021 · 8 comments
Labels

Comments

@DanielOrtel
Copy link
Contributor

Describe the bug

I'm honestly not sure if the culprit is this package or nextJS, but this seems like the most likely candidate. Anyway, I have a weird issue in that hot reload is broken for reducers, and only reducers. Everything else hot reloads on change, except reducers.

Possible related issue from NextJS, where their own with-redux-thunk example had the exact same issue:
vercel/next.js#11794

And their fix:
vercel/next.js#11816

To Reproduce

  1. Create a new project with npx create-next-app, add redux, next-redux-wrapper, set up per the docs, with no additional bells or whistles(redux-devtools-extension makes no difference either way).
  2. Create a generic todo reducer
  3. Modify todo reducer
  4. Changes are not reflected on hot reload, needs a manual tab reload to work.

Expected behavior

Hot reloading for reducers works out of the box, without any additional steps.

Screenshots

N/A

Desktop (please complete the following information):

  • OS: OSX Catalina, but probably N/A
  • Browser: chrome, but probably N/A
  • Version: 6.0.2

Additional context

If you want I can push an example app for this.

@kirill-konshin
Copy link
Owner

There's nothing I can do about it on my end.

@DanielOrtel
Copy link
Contributor Author

@kirill-konshin meaning is this something that should be raised with the folks over at nextJS?

@kirill-konshin
Copy link
Owner

kirill-konshin commented Jan 12, 2021

It depends a lot on how the hot reload of reducer is implemented. You still need to have module.hot.accept call with replace reducer code, similar to this: https://stackoverflow.com/a/34697765/5125659

https://github.com/kirill-konshin/next-redux-wrapper/blob/master/packages/demo/src/components/store.tsx see the demo in this repo

@DanielOrtel
Copy link
Contributor Author

Explicitly requiring hot reloading, even when redux devtools were added, was never the case on any one project I worked on. In fact, NextJS's own fixed redux example has functioning hot reloading for reducers without ever having to manually call module.hot.accept on the reducer paths.

This is an issue with this package specifically. It's not feasible for projects to manually do this, especially with larger ones with reducers splattered all over the place, even external packages.

@DanielOrtel
Copy link
Contributor Author

DanielOrtel commented Feb 10, 2021

@kirill-konshin after some investigation, I've pinpointed the issue: adding the redux store to the window here.

Is this absolutely necessary? If we wanted to memoize, wouldn't an explicit useMemo in Wrapper be a better solution?

I'd open a PR for this, but I'm honestly confused about why the store is assigned to a ref there.

@kirill-konshin
Copy link
Owner

@DanielOrtel thank you for investigation, good catch. It was created long ago, before useMemo hooks. useRef basically does the same thing as useMemo since I assign external object, that never changes, just adds more semantic usage. If you can make a PR — go for it! But use types-enhancements branch, since it's 7.x release candidate. https://github.com/kirill-konshin/next-redux-wrapper/blob/types-enchancements/packages/wrapper/src/index.tsx — I had to rework functional component back to class component since functional can't make a synchronous dispatch during render.

@DanielOrtel
Copy link
Contributor Author

alright, I'll give it a whirl, I also noticed another thing while investigating this, but I'll make a different issue for that, since it's not directly related

@DanielOrtel
Copy link
Contributor Author

nvm the other thing, it was fixed in the types-enhancement.

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

No branches or pull requests

2 participants