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

fix: avoid reloading all csses when hot load #1090

Merged
merged 3 commits into from Apr 16, 2024

Conversation

yiminghe
Copy link
Contributor

@yiminghe yiminghe commented Mar 20, 2024

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

  • only refresh page when locals change, otherwise accept and hot load
  • avoid reloading all csses

Breaking Changes

Additional Info

Fixes #692

Copy link

linux-foundation-easycla bot commented Mar 20, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: yiminghe / name: yiminghe (9bfc65e)
  • ✅ login: alexander-akait / name: Alexander Akait (ace213e, fc80832)

? ""
: "module.hot.accept(undefined, cssReload);";
Copy link
Member

Choose a reason for hiding this comment

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

Why you change it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

self accept is enough, it will update when dispose: https://webpack.js.org/api/hot-module-replacement/

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

We need a test case, please add

@yiminghe yiminghe changed the title fix: avoid reload all css when hot load fix: avoid reloading all csses when hot load Mar 20, 2024
@yiminghe yiminghe force-pushed the optimize-locals branch 4 times, most recently from ea2c589 to bff1a98 Compare March 21, 2024 04:39
@yiminghe
Copy link
Contributor Author

We need a test case, please add

test cases updated

src/loader.js Outdated
// ${Date.now()}
var cssReload = require(${stringifyRequest(
const cssReload = require(${stringifyRequest(
Copy link
Member

Choose a reason for hiding this comment

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

Don't use const, it will be in the browser and break old browsers

? ""
: "module.hot.accept(undefined, cssReload);";

const localsJsonString = JSON.stringify(JSON.stringify(context.locals));
Copy link
Member

Choose a reason for hiding this comment

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

Why double JSON.stringify? Just typo?

Copy link
Contributor Author

@yiminghe yiminghe Mar 28, 2024

Choose a reason for hiding this comment

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

note the interpolation:

`const localsJsonString = ${localsJsonString};`

string comparation is faster

Copy link

codecov bot commented Mar 27, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 92.00%. Comparing base (c7ff30d) to head (9bfc65e).
Report is 7 commits behind head on master.

❗ Current head 9bfc65e differs from pull request most recent head fc80832. Consider uploading reports for the commit fc80832 to get more accurate results

Files Patch % Lines
src/loader.js 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1090      +/-   ##
==========================================
+ Coverage   90.61%   92.00%   +1.38%     
==========================================
  Files           5        5              
  Lines         895      900       +5     
  Branches      255      258       +3     
==========================================
+ Hits          811      828      +17     
+ Misses         74       67       -7     
+ Partials       10        5       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

module.hot.dispose(function(data) {
data.value = localsJsonString;
cssReload();
});
Copy link
Member

Choose a reason for hiding this comment

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

We have a special test directory for HMR, will be great to add a test case there, I know your code works fine, but we should ensure about it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you mean? I have added cases inside cases folder (hmr, hmr-locals)

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Let's do small improvements and I am fine with it

@alexander-akait
Copy link
Member

For fixing tests for OLD API just use OLD_API=1 npm run test:only

@alexander-akait
Copy link
Member

Thank you for your PR

@yiminghe
Copy link
Contributor Author

For fixing tests for OLD API just use OLD_API=1 npm run test:only

passed

@alexander-akait
Copy link
Member

alexander-akait commented Apr 8, 2024

@yiminghe We have:

var cssReload = require("/home/runner/work/mini-css-extract-plugin/mini-css-extract-plugin/src/hmr/hotModuleReplacement.js")(module.id, undefined);

As you can see it is an absolute path, let's use it from here https://github.com/webpack-contrib/css-loader/blob/master/src/utils.js#L15 (maybe we already have the same util here)

@yiminghe
Copy link
Contributor Author

yiminghe commented Apr 9, 2024

@yiminghe We have:

var cssReload = require("/home/runner/work/mini-css-extract-plugin/mini-css-extract-plugin/src/hmr/hotModuleReplacement.js")(module.id, undefined);

As you can see it is an absolute path, let's use it from here https://github.com/webpack-contrib/css-loader/blob/master/src/utils.js#L15 (maybe we already have the same util here)

updated

alexander-akait
alexander-akait previously approved these changes Apr 9, 2024
@alexander-akait alexander-akait merged commit 1a56673 into webpack-contrib:master Apr 16, 2024
14 of 33 checks passed
@yiminghe yiminghe deleted the optimize-locals branch April 17, 2024 03:21
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