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

Enable build cache and HMR #2656

Merged
merged 3 commits into from
Nov 3, 2022
Merged

Enable build cache and HMR #2656

merged 3 commits into from
Nov 3, 2022

Conversation

dlabrecq
Copy link
Contributor

@dlabrecq dlabrecq commented Nov 1, 2022

Enable build cache and HMR.

https://issues.redhat.com/browse/COST-3220

@Hyperkid123
Copy link
Contributor

@dlabrecq ok I was able to get it working. The issues is with direct default exports. See: https://github.com/pmmmwh/react-refresh-webpack-plugin/blob/main/docs/TROUBLESHOOTING.md#edits-always-lead-to-full-reload

This diff made it work for me:

diff --git a/src/app.tsx b/src/app.tsx
index aa66f29e..61526146 100644
--- a/src/app.tsx
+++ b/src/app.tsx
@@ -13,7 +13,7 @@ export interface AppOwnProps {
   basename: string;
 }
 
-interface AppStateProps {}
+interface AppStateProps { }
 
 interface AppDispatchProps {
   history: any;
@@ -88,4 +88,8 @@ const mapStateToProps = createMapStateToProps<AppOwnProps, AppStateProps>((state
 
 const mapDispatchToProps: AppDispatchProps = { history };
 
-export default compose<React.ComponentType<AppOwnProps>>(withRouter, connect(mapStateToProps, mapDispatchToProps))(App);
+const ComposedApp = compose<React.ComponentType<AppOwnProps>>(
+  withRouter,
+  connect(mapStateToProps, mapDispatchToProps)
+)(App);
+export default ComposedApp;
diff --git a/src/appEntry.tsx b/src/appEntry.tsx
index 76f63d39..bbdeca52 100644
--- a/src/appEntry.tsx
+++ b/src/appEntry.tsx
@@ -29,7 +29,7 @@ const store = configureStore({
   // },
 });
 
-export default () => {
+const AppEntry = () => {
   const basename = getBaseName(window.location.pathname);
   const locale = getLocale();
 
@@ -46,3 +46,5 @@ export default () => {
     </div>
   );
 };
+
+export default AppEntry;

@Hyperkid123
Copy link
Contributor

It is possible that making changes to some nested components that follow the same pattern might cause a full refresh. But making sure the function is not directly exported as a default module will fix the potential issue.

@codecov-commenter
Copy link

codecov-commenter commented Nov 2, 2022

Codecov Report

Merging #2656 (b75de5c) into main (413e764) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##             main    #2656    +/-   ##
========================================
  Coverage   65.70%   65.70%            
========================================
  Files         451      451            
  Lines        8918     8918            
  Branches     2081     1937   -144     
========================================
  Hits         5860     5860            
  Misses       3055     3055            
  Partials        3        3            

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 413e764...b75de5c. Read the comment docs.

@dlabrecq
Copy link
Contributor Author

dlabrecq commented Nov 2, 2022

Thank you for the help, Martin.

I wasn't able to see HMR working with the diffs, so I temporarily disabled the feature. Just going to use the build cache for now.

I created an issue to refactor our unnamed default exports in order to enable HMR separately.
https://issues.redhat.com/browse/COST-3224

@dlabrecq dlabrecq marked this pull request as ready for review November 2, 2022 14:24
@dlabrecq dlabrecq requested review from gitdallas and removed request for gitdallas November 2, 2022 14:25
@dlabrecq dlabrecq merged commit 1abae43 into project-koku:main Nov 3, 2022
@dlabrecq dlabrecq deleted the 3220-hmr branch November 3, 2022 14:00
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.

3 participants