fix: css url public path by ahabhgk · Pull Request #17170 · webpack/webpack

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


+				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?

alexander-akait · 2024-06-10T13:51:29Z

@ahabhgk Can you rebase, thank you ⭐

alexander-akait · 2024-06-10T14:40:55Z

Thank you, will look soon and we will make a release ⭐

alexander-akait · 2024-06-10T19:03:29Z

Thank you, release will be tomorrow with a lot of fixes and features

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 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

alexander-akait reviewed Mar 18, 2024

View reviewed changes

alexander-akait mentioned this pull request Jun 5, 2024

fix: mangle with destructuring #18319

Merged

ahabhgk added 4 commits June 10, 2024 22:29

fix: css url public path

c561748

works for node10

4a360f6

rebase and test

673278c

fix module rule output path

3afed06

ahabhgk added 3 commits June 10, 2024 22:33

add css render module cache

0b92e07

rebase

944547e

remove toString

9ed5255

ahabhgk force-pushed the fix-css-url-public-path2 branch from 5be76c5 to 9ed5255 Compare June 10, 2024 14:35

lint

eb534d5

alexander-akait approved these changes Jun 10, 2024

View reviewed changes

webpack-bot added PR: reviewed-approved and removed PR: review-outdated labels Jun 10, 2024

alexander-akait merged commit 0c5879a into webpack:main Jun 10, 2024

SoonIter mentioned this pull request Dec 24, 2024

[Bug]: wrong url path when using generator.publicPath and asset module webpack/mini-css-extract-plugin#1118

Closed

Uh oh!

Conversation

ahabhgk commented May 10, 2023 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

🤖 Generated by Copilot at 33215ce

Details

🤖 Generated by Copilot at 33215ce

Uh oh!

webpack-bot commented May 10, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-akait left a comment

Choose a reason for hiding this comment

Uh oh!

alexander-akait commented Mar 13, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-akait left a comment

Choose a reason for hiding this comment

Uh oh!

alexander-akait commented Mar 13, 2024

Uh oh!

alexander-akait commented Mar 13, 2024

Uh oh!

webpack-bot commented Mar 15, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-akait left a comment

Choose a reason for hiding this comment

Uh oh!

alexander-akait commented Apr 8, 2024

Uh oh!

alexander-akait commented Apr 12, 2024

Uh oh!

alexander-akait commented Jun 10, 2024

Uh oh!

alexander-akait commented Jun 10, 2024

Uh oh!

alexander-akait commented Jun 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ahabhgk commented May 10, 2023 •

edited by ghost

Loading

`🤖 Generated by Copilot at 33215ce`

`🤖 Generated by Copilot at 33215ce`