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

Minify also the HTML files #693

Merged
merged 1 commit into from
Aug 9, 2023
Merged

Minify also the HTML files #693

merged 1 commit into from
Aug 9, 2023

Conversation

lslezak
Copy link
Contributor

@lslezak lslezak commented Aug 8, 2023

Problem

Testing

  • Tested manually
    • Running the development server locally (npm run server), I also tested Hot Module Replacement feature by editing some .jsx file
    • Building the production code (NODE_ENV=production npm run build) and copying the dist/* files to a testing machine
    • Building the development code (NODE_ENV=development npm run build && gzip dist/index.*) and copying the dist/* files to a testing machine

In all cases it worked fine. 😇

Notes

  • That reactRefreshSetup should not be needed anymore, I haven't found any reference to it in the documentation.

@imobachgs
Copy link
Contributor

I tested the code and it works for me too. However, as we do not have that much HTML code, the main change is that the reactRefreshSetup* files are missing:

Before:

4,0K    dist/favicon.svg
888K    dist/fonts
768K    dist/index.css
1,1M    dist/index.css.map
4,0K    dist/index.html
9,5M    dist/index.js
9,1M    dist/index.js.map
4,0K    dist/manifest.json
4,0K    dist/po.cs.js
84K     dist/reactRefreshSetup.js
68K     dist/reactRefreshSetup.js.map

After:

4,0K    dist/favicon.svg
888K    dist/fonts
768K    dist/index.css
1,1M    dist/index.css.map
4,0K    dist/index.html
9,5M    dist/index.js
9,1M    dist/index.js.map
4,0K    dist/manifest.json
4,0K    dist/po.cs.js

@lslezak
Copy link
Contributor Author

lslezak commented Aug 9, 2023

That missing reactRefreshSetup.js is fine and IMHO it is a good idea to remove it.

  • This file should not be distributed in production builds (as it is so far). If you try it loading in the production code you will get "React Refresh runtime should not be included in the production bundle" error
  • I haven't found any reference to that file in it's documentation, the setup instructions do not mention that file at all
  • The built index.js file includes that file in the development mode (grep ReactRefreshEntry.js dist/index.js)
  • The hot module replacement still works fine without it

My guess is that it was probably needed in some older react-refresh version or without using the react-refresh-webpack-plugin...

@lslezak lslezak merged commit 4df4c55 into master Aug 9, 2023
14 checks passed
@lslezak lslezak deleted the html_minimizer branch August 9, 2023 08:24
@imobachgs imobachgs mentioned this pull request Sep 26, 2023
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.

2 participants