fix: css url public path #17170

ahabhgk · 2023-05-10T11:08:36Z

Summary

`🤖 Generated by Copilot at 33215ce`

This pull request adds a new feature to the CSS modules plugin that allows using a placeholder for the public path in the CSS source and dependencies. This enables the public path to be resolved at runtime instead of being hardcoded in the generated CSS assets. It also adds a test case to verify the functionality and refactors some code for clarity and consistency.

Details

`🤖 Generated by Copilot at 33215ce`

Import ReplaceSource class and getUndoPath function to enable replacing public path placeholder in CSS module source (link, link)
Obtain filename and info properties of CSS asset by calling compilation.getPathWithInfo and compute publicPath property using getUndoPath or compilation.getAssetPath (link)
Replace filenameTemplate and pathOptions properties of CSS asset with filename, info, publicPath, and modules properties (link)
Add publicPath parameter to renderContentAsset method of CssModulesPlugin class and wrap moduleSource in ReplaceSource instance to replace public path placeholder (link, link, link)
Define PUBLIC_PATH_PLACEHOLDER constant as a special string to mark public path in CSS url dependencies and use it in runtimeTemplate.assetUrl calls in CssUrlDependencyTemplate class (link, link, link, link)
Add test case to test/configCases/css/urls-css-filename/index.js to check styles in div.css (link)
Add nested.css file to test/configCases/css/urls-css-filename/ to test nested import and url handling (link)
Add webpack.config.js file to test/configCases/css/urls-css-filename/ to set up output options, CSS experiments, and split chunks optimization for test case (link)

webpack-bot · 2023-05-10T11:08:40Z

For maintainers only:

This needs to be documented (issue in webpack/webpack.js.org will be filed when merged)
This needs to be backported to webpack 4 (issue will be created when merged)

alexander-akait · 2023-05-11T08:27:04Z

lib/css/CssModulesPlugin.js

@@ -501,6 +519,21 @@ class CssModulesPlugin {
 					codeGenResult.sources.get("css") ||
 					codeGenResult.sources.get("css-import");

+				const moduleSourceCode = moduleSource.source().toString();
+				const publicPathRegex = new RegExp(
+					CssUrlDependency.PUBLIC_PATH_PLACEHOLDER,


It breaks source maps, we should not do it, we should rewrite assets modules and generate the valid URL there

Yes, we use the same logic in mini-css-extract-plugin, but it was wrong and yeah our source maps are broken in some cases, there are issues about it

Is there any link about the source maps issues, would like to learn more.

Just make an URL with a newline and put somethings CSS on the same line.

Also if you look your bundled JS file, it will contain asset module, this is wrong, so the solution is refactor assets module and make it compatibility with CSS

Good to know, thanks

alexander-akait

Sorry, wrong solution

alexander-akait · 2024-03-13T17:17:11Z

@ahabhgk I tried different solution and I think I found. We have two problems here:

When we have an asset only in CSS module we include runtime for loading assets modules in JS bundle, but there is no asset module for loading in JS, we should fix it, I will do it in a separate PR
Your solution is not bad, we need to improve path using getUndoPath (so we need to do it late), I will finish your PR soon too

Time to finish CSS feature

alexander-akait · 2024-03-13T17:18:00Z

lib/css/CssModulesPlugin.js

+						(match.index += match[0].length - 1),
+						publicPath
+					);
+				}


Ideally we need to use ReplaceSource here for caching, just an idea

Did you mean CachedSource?

https://github.com/webpack/webpack-sources?tab=readme-ov-file#replacesource

Oh I see, add cache for css renderModule result just like _moduleFactoryCache added in js renderModule

Yeah, feel free to try it, should work without perf and source maps problems

alexander-akait

And we need more test:

When asset above CSS file
When asset on the same level
When asset in nested directory

Each test should be for public path auto public path with string value

Also we have https://webpack.js.org/configuration/module/#rulegeneratoroutputpath, so we need to add a test case too

alexander-akait · 2024-03-13T17:21:29Z

A lot of tests... If you have time you can add them to your solution to ensure the idea is working

alexander-akait · 2024-03-13T17:21:37Z

And feel free to rebase

webpack-bot · 2024-03-15T13:01:30Z

@ahabhgk Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@alexander-akait Please review the new changes.

alexander-akait · 2024-03-17T13:48:50Z

lib/css/CssModulesPlugin.js

+		) {
+			source = cacheEntry.source;
+		} else {
+			const moduleSourceCode = moduleSourceContent.source().toString();


Do we need to use .toString()? It is very bad for perf

alexander-akait · 2024-03-17T13:52:17Z

lib/asset/AssetGenerator.js

+											hash: compilation.hash
+										}
+									);
+						assetPathForCss = path + filename;


Maybe we can generate this only when asset module in CSS dependecy to avoid extra work, because we still need to improve it, have
runtimeRequirements.add(RuntimeGlobals.publicPath);, and so we generate extra JS code if you have an asset in your CSS (but not in JS), and in future we will need to fix it too (I described it above)

Yes, both need the asset module to aware whether it is only referenced by a css module or a js module, since it's not very related to this PR, I can give a try in the future PR

alexander-akait

Thank you look good, I want to make some investigation about perf and more test cases (I will do it), so I will finish and merge it for the next release ❤️

alexander-akait · 2024-04-08T16:21:20Z

I want to make a release on this week with this fix, so don't worry, it is not abadoned, will review in the next few days

alexander-akait · 2024-04-12T12:11:18Z

/cc @vankop Can you look at this too?

webpack-bot added the PR: unreviewed label May 10, 2023

alexander-akait reviewed May 11, 2023

View reviewed changes

alexander-akait requested changes May 11, 2023

View reviewed changes

webpack-bot added PR: reviewed-changes-requested and removed PR: unreviewed labels May 11, 2023

webpack-bot added the PR: conflict label Jun 22, 2023

alexander-akait reviewed Mar 13, 2024

View reviewed changes

ahabhgk force-pushed the fix-css-url-public-path2 branch from 33215ce to c0c1ac8 Compare March 15, 2024 13:01

webpack-bot added PR: review-outdated and removed PR: conflict PR: reviewed-changes-requested labels Mar 15, 2024

ahabhgk added 6 commits March 16, 2024 19:41

fix: css url public path

ad4e172

works for node10

e02e2d0

rebase and test

2ac2978

fix module rule output path

ef79fac

add css render module cache

98144ad

rebase

b64d1a5

ahabhgk force-pushed the fix-css-url-public-path2 branch from 1af0127 to b64d1a5 Compare March 16, 2024 11:52

ahabhgk requested a review from alexander-akait March 16, 2024 12:23

alexander-akait reviewed Mar 17, 2024

View reviewed changes

remove toString

5be76c5

alexander-akait reviewed Mar 18, 2024

View reviewed changes

alexander-akait mentioned this pull request Jun 5, 2024

fix: mangle with destructuring #18319

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: css url public path #17170

fix: css url public path #17170

ahabhgk commented May 10, 2023 •

edited by ghost

webpack-bot commented May 10, 2023

alexander-akait May 11, 2023

alexander-akait May 11, 2023

ahabhgk May 11, 2023

alexander-akait May 14, 2023

ahabhgk May 15, 2023

alexander-akait left a comment

alexander-akait commented Mar 13, 2024

alexander-akait Mar 13, 2024

ahabhgk Mar 15, 2024

alexander-akait Mar 15, 2024

ahabhgk Mar 15, 2024

alexander-akait Mar 15, 2024

alexander-akait left a comment

alexander-akait commented Mar 13, 2024

alexander-akait commented Mar 13, 2024

webpack-bot commented Mar 15, 2024

alexander-akait Mar 17, 2024

alexander-akait Mar 17, 2024

ahabhgk Mar 18, 2024

alexander-akait left a comment

alexander-akait commented Apr 8, 2024

alexander-akait commented Apr 12, 2024

fix: css url public path #17170

Are you sure you want to change the base?

fix: css url public path #17170

Conversation

ahabhgk commented May 10, 2023 • edited by ghost

Summary

🤖 Generated by Copilot at 33215ce

Details

🤖 Generated by Copilot at 33215ce

webpack-bot commented May 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-akait left a comment

Choose a reason for hiding this comment

alexander-akait commented Mar 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-akait left a comment

Choose a reason for hiding this comment

alexander-akait commented Mar 13, 2024

alexander-akait commented Mar 13, 2024

webpack-bot commented Mar 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-akait left a comment

Choose a reason for hiding this comment

alexander-akait commented Apr 8, 2024

alexander-akait commented Apr 12, 2024

ahabhgk commented May 10, 2023 •

edited by ghost

`🤖 Generated by Copilot at 33215ce`

`🤖 Generated by Copilot at 33215ce`