plone / volto

React-based frontend for the Plone Content Management System
https://demo.plone.org/
MIT License
426 stars 576 forks source link

less modules not working on Volto 17 #5019

Closed wesleybl closed 7 months ago

wesleybl commented 9 months ago

I have an addon that uses less modules. In Volto 16, the CSS is generated normally. In Volto 17, in production mode, the less CSS is not added to the bundle. The strange thing is that in development mode everything works ok. Does anyone have any idea what might be going on? I also tried CSS modules and SCSS modules and had the same problem.

davisagli commented 9 months ago

I don't know the specific reason, but here are some things to look at:

  1. differences between how CSS is loaded in dev and production mode -- search for production in https://github.com/plone/volto/blob/master/src/helpers/Html/Html.jsx
  2. the webpack plugin to handle LESS https://github.com/plone/volto/blob/master/webpack-plugins/webpack-less-plugin.js
  3. Volto 17 uses webpack 5 instead of webpack 4. Make sure you don't have webpack-related packages pinned to a different version in your addon, compared to the versions that volto uses
wesleybl commented 9 months ago

@davisagli looking closer, I saw that the CSS is added but the code generated at the end of the class name is different between the html and the CSS. For example, in the html we have the class:

Footer-module__footer___VgK5e

and in CSS we have:

Footer-module__footer___Kio8a

davisagli commented 9 months ago

Did you upgrade the razzle version in your addon's package.json as mentioned in the upgrade guide? https://github.com/plone/volto/blob/master/docs/source/upgrade-guide/index.md#razzle-upgraded-to-version-4218

(Not sure if that's related, but it does sound like something that could potentially be caused if different versions of some package are being used in different places)

wesleybl commented 9 months ago

Did you upgrade the razzle version in your addon's package.json as mentioned in the upgrade guide?

Yes I did it. I even generated a new Volto app with the generator.

wesleybl commented 9 months ago

I rolled back versions of Volto and the problem started to occur on Volto 17.0.0-alpha.7. On Volto 17.0.0-alpha.6 it works. So something in 17.0.0-alpha.7 broke CSS modules.

wesleybl commented 9 months ago

In Changelog of version 17.0.0-alpha.7 we only have:

Fix language negotiation for language codes that include a region (e.g. pt-br). @davisagli https://github.com/plone/volto/issues/4644

This doesn't seem to be related.

Although my site is in pt-br.

davisagli commented 9 months ago

@wesleybl And that change was also backported to Volto 16 (https://github.com/plone/volto/pull/4755).

Really strange.

wesleybl commented 9 months ago

@davisagli On Volto 16 with this backport it works. The only thing left in alpha 7 is this commit from @sneridagh :

https://github.com/plone/volto/commit/7e0a6fe8d3e832826ced8c96c31dc442654df94a

Do you think it might be related?

tiberiuichim commented 9 months ago

@wesleybl "less modules" may be related to razzle plugins configuration.

tiberiuichim commented 9 months ago

https://github.com/plone/volto/blob/03cd36dfa82de3a03b2f88a6b59ceeea1bc99292/webpack-plugins/webpack-less-plugin.js#L44-L46

tiberiuichim commented 9 months ago

maybe webpack needs similar for the less plugin.

wesleybl commented 9 months ago

@wesleybl "less modules" may be related to razzle plugins configuration.

@tiberiuichim that was my first suspicion, but I discarded it. I replaced in Volto 17, the webpack-less-plugin.js and razzle.config.js files, at the corresponding files of Volto 16 and had the same problem.

What's more, as I said in https://github.com/plone/volto/issues/5019#issuecomment-1648445032, it worked on Volto 17 alpha 6.

My biggest suspicion is what I said in https://github.com/plone/volto/issues/5019#issuecomment-1648554176.

wesleybl commented 9 months ago

@rotavio FYI.

wesleybl commented 9 months ago

I pasted the ignored files in https://github.com/plone/volto/commit/7e0a6fe8d3e832826ced8c96c31dc442654df94a into node_modules/@plone/volto and the problem continued. So the problem doesn't seem to be this commit.

wesleybl commented 9 months ago

I'm going to create a repository with a Volto app simulating the problem, so that other people can see the problem.

wesleybl commented 9 months ago

@davisagli @tiberiuichim I created this repository to demonstrate the problem.

I customized the View.jsx to insert these components:

https://github.com/wesleybl/cssmodules.volto/blob/main/src/components/CssModules/CssModules.jsx

It uses a simple CSS modules here:

https://github.com/wesleybl/cssmodules.volto/blob/main/src/components/CssModules/CssModules.jsx#L4

This will make the text This text should be red appear on the screen.

if you use the command:

yarn build && yarn start:prod

the text does not turn red.

Any tip is welcome.

wesleybl commented 9 months ago

Strange is that in this repository that I created, the error occurs even if I pin the alpha 6 version.

davisagli commented 9 months ago

@wesleybl Thanks for putting together the repro repository. What version of Node should I try it with to match your own testing?

davisagli commented 9 months ago

@wesleybl ~Interesting, I can reproduce the problem with Node 16, but it works with Node 18.~

I may have been on the wrong track. In both Node 16 and 18, it works when the html is rendered in the browser, but not when I refresh and get the html from SSR

wesleybl commented 9 months ago

What version of Node should I try it with to match your own testing?

Node 18

wesleybl commented 9 months ago

In Node 16, it works when the html is rendered in the browser, but not when I refresh and get the html from SSR

I'm always pressing f5 to test.

wesleybl commented 9 months ago

@davisagli could it be that the javascript is rewriting the class name, which comes from the server, putting an id that does not exist in the CSS?

davisagli commented 9 months ago

@wesleybl It seems to me that the problem is that the css-loader module is generating a different hash when it loads the module for the server bundle and for the client CSS bundle. So far I am not sure why.

wesleybl commented 9 months ago

@davisagli I took a closer look at this problem and found the following. The difference between Volto 16 and Volto 17 (which here is basically the difference between webpack 4 and webpack 5) is as follows. In Volto 16, loaderContext._module.matchResource here:

https://github.com/webpack-contrib/css-loader/blob/v5.2.7/src/utils.js#L252

for both the server and client builds, it comes empty. In Volto 17, it is filled in the client build and empty in the server build. Being filled in or not influences hash generation. Something has changed in Volto 17/webpack 5, which means that this variable is filled in when compiling the client. It looks like this variable is set here:

https://github.com/webpack/webpack/blob/v5.76.1/lib/NormalModuleFactory.js#L364-L366

In server compilation, it doesn't go into that if.

I don't know what makes webpack 5 set this variable only on client compilation. Would it be some configuration different between the compilation of the client and the server? Where is this configuration done? Would it be razzle?

wesleybl commented 7 months ago

I found out the webpack version this started to occur on. It was in version 5.52.0. More specifically in this commit:

https://github.com/webpack/webpack/commit/765101bded6bf9dc9693fac6af18771d11b37eed

made by @sokra

This commit was made in PR:

https://github.com/webpack/webpack/pull/14134

To fix a problem in detected in mini-css-extract-plugin:

https://github.com/webpack-contrib/mini-css-extract-plugin/issues/755

So, after this commit, if mini-css-extract-plugin is used, webpack started to fill the matchResource variable (which is used in hashing the CSS class name).

But Volto use mini-css-extract-plugin only in the client build:

https://github.com/plone/volto/blob/6ed97685a90d15014b9c13cc2f7a4db3791b1659/webpack-plugins/webpack-less-plugin.js#L143

So the hash of the class name looks different on client and server.

The function that generates the hash can be customized. I did it in commit:

https://github.com/plone/volto/commit/10168c3a7aae6ec5c37436b9037cb3255ec93520

In this custom function, I disregarded the matchResource variable when generating the hash. Then it worked. This commit I also put the text in red with css modules to test.

Is there a better solution? Should I do a PR? How do I add this setting to a Volto app?

@davisagli @tiberiuichim @sneridagh @sokra

tiberiuichim commented 7 months ago

@wesleybl I think the fix is valid, it should be integrated. The only suggestion I have is to slightly rewrite the webpack-less-plugin.js additions, to make them more in line with the rest of the code, I presume you have copied the code from transpiled code.

wesleybl commented 7 months ago

@tiberiuichim I did PR #5165 , changing the code as you suggested.