webpack-contrib/mini-css-extract-pluginPublic

NotificationsYou must be signed in to change notification settings
Fork388
Star4.7k

fix: avoid reloading all csses when hot load#1090

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

alexander-akait merged 3 commits intowebpack-contrib:masterfromyiminghe:optimize-locals

Apr 16, 2024

Merged

fix: avoid reloading all csses when hot load#1090

alexander-akait merged 3 commits intowebpack-contrib:masterfromyiminghe:optimize-locals

Apr 16, 2024

Conversation

Copy link

Contributor

yiminghe commentedMar 20, 2024•
edited by alexander-akait
Loading

This PR contains a:

bugfix
newfeature
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-easyclabot commentedMar 20, 2024•
edited
Loading

The committers listed above are authorized under a signed CLA.

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

alexander-akait reviewed

Mar 20, 2024

View reviewed changes

src/loader.js

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

Copy link

Member

alexander-akaitMar 20, 2024•
edited
Loading

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

ContributorAuthor

yimingheMar 20, 2024

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/

alexander-akait reviewed

Mar 20, 2024

View reviewed changes

Copy link

Member

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 changed the title~~fix: avoid reload all css when hot load~~fix: avoid reloading all csses when hot load

Mar 20, 2024

Copy link

ContributorAuthor

yiminghe commentedMar 21, 2024

We need a test case, please add

test cases updated

alexander-akait reviewed

Mar 27, 2024

View reviewed changes

src/loader.js Outdated

		// ${Date.now()}
		var cssReload = require(${stringifyRequest(
		const cssReload = require(${stringifyRequest(

Copy link

Member

alexander-akaitMar 27, 2024

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

alexander-akait reviewed

Mar 27, 2024

View reviewed changes

src/loader.js

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

		const localsJsonString = JSON.stringify(JSON.stringify(context.locals));

Copy link

Member

alexander-akaitMar 27, 2024

Choose a reason for hiding this comment

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

Why doubleJSON.stringify? Just typo?

Copy link

ContributorAuthor

yimingheMar 28, 2024•
edited
Loading

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

codecovbot commentedMar 27, 2024•
edited
Loading

Codecov Report

Attention: Patch coverage is50.00000% with1 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 head9bfc65e differs from pull request most recent headfc80832. Consider uploading reports for the commitfc80832 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.

alexander-akait reviewed

Mar 27, 2024

View reviewed changes

test/cases/hmr/expected/main.js Outdated

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

Copy link

Member

alexander-akaitMar 27, 2024

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

ContributorAuthor

yimingheMar 28, 2024

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

alexander-akaitApr 2, 2024

Choose a reason for hiding this comment

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

Sorry for delay, just put a test case herehttps://github.com/webpack-contrib/mini-css-extract-plugin/blob/master/test/HMR.test.js

Copy link

ContributorAuthor

yimingheApr 3, 2024

Choose a reason for hiding this comment

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

Sorry for delay, just put a test case herehttps://github.com/webpack-contrib/mini-css-extract-plugin/blob/master/test/HMR.test.js

done

alexander-akait requested changes

Mar 27, 2024

View reviewed changes

Copy link

Member

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

Copy link

Member

alexander-akait commentedMar 27, 2024

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

Copy link

Member

alexander-akait commentedMar 27, 2024

Thank you for your PR

Copy link

ContributorAuthor

yiminghe commentedMar 28, 2024

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

passed

Copy link

Member

alexander-akait commentedApr 8, 2024•
edited
Loading

@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 herehttps://github.com/webpack-contrib/css-loader/blob/master/src/utils.js#L15 (maybe we already have the same util here)

fix: avoid reload all css when hot load

9bfc65e

Copy link

ContributorAuthor

yiminghe commentedApr 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 herehttps://github.com/webpack-contrib/css-loader/blob/master/src/utils.js#L15 (maybe we already have the same util here)

updated