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

restructure: Export Redux store from separate file #1059

Merged

Conversation

hyperupcall
Copy link
Contributor

Description

Defining the Redux store in the same file as the app will create problems with Vite. There is some issue with circular imports not working properly with HMR (hot module replacement). Upstream tracks this issue at vitejs/vite#3033; a workaround is to define and export the Redux store from a different file. This commit does exactly that to circumvent the circular import issue.

Related to #879

Type of change

  • Note merging this changes the database configuration.
  • This change requires a documentation update

Checklist

  • I have followed the OED pull request ideas
  • I have removed text in ( ) from the issue request
  • You acknowledge that every person contributing to this work has signed the OED Contributing License Agreement and each author is listed in the Description section.

Limitations

N/A

Defining the Redux store in the same file as the app will create problems
with Vite. There is some issue with circular imports not working
properly with HMR (hot module replacement). Upstream tracks this issue
at vitejs/vite#3033; a workaround is to define and export the Redux
store from a different file. This commit does exactly that to circumvent
the circular import issue.
Copy link
Member

@huss huss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks to @hyperupcall for your continued support of OED and this PR. I reviewed the code and everything seems fine. Testing did not find any issues. I have two minor comments about the need for imports that I would like your input on (welcome to change if appropriate). Once that is resolved this is ready to merge.

src/client/app/store.ts Outdated Show resolved Hide resolved
src/client/app/store.ts Outdated Show resolved Hide resolved
@huss
Copy link
Member

huss commented Nov 7, 2023

Thanks to @hyperupcall for the quick response. Everything looks goos and this is ready to merge.

@huss huss merged commit 3d6d83a into OpenEnergyDashboard:development Nov 7, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants