Skip to content

Drop support of css-loader ^6, add support for css-loader ^7.1#1319

Merged
Kocal merged 2 commits intosymfony:mainfrom
Kocal:chore/css-loader-7
Sep 25, 2024
Merged

Drop support of css-loader ^6, add support for css-loader ^7.1#1319
Kocal merged 2 commits intosymfony:mainfrom
Kocal:chore/css-loader-7

Conversation

@Kocal
Copy link
Member

@Kocal Kocal commented Sep 1, 2024 •
edited
Loading

Let's continue to upgrade major dependencies, see https://github.com/webpack-contrib/css-loader/blob/master/CHANGELOG.md#700-2024-04-04 for changelog

@Kocal Kocal added this to the 5.0 milestone Sep 1, 2024
@Kocal
Copy link
Member Author

Kocal commented Sep 1, 2024

Hum, looks like there is more work to do

@stof
Copy link
Member

stof commented Sep 2, 2024

why is the title taking about webpack-dev-server ?

@Kocal Kocal changed the title Drop support of css-loader ^6, add support for webpack-dev-server ^7.1 Drop support of css-loader ^6, add support for css-loader ^7.1 Sep 2, 2024
@Kocal Kocal force-pushed the chore/css-loader-7 branch 2 times, most recently from 8ab29a1 to ac94831 Compare September 2, 2024 15:14
@Kocal
Copy link
Member Author

Kocal commented Sep 2, 2024

why is the title taking about webpack-dev-server ?

Fixed, thanks

@Kocal Kocal force-pushed the chore/css-loader-7 branch from ac94831 to 3e83aa0 Compare September 2, 2024 20:24
@Kocal Kocal marked this pull request as draft September 3, 2024 06:03
@Kocal
Copy link
Member Author

Kocal commented Sep 3, 2024

Let's keep in draft for the moment. I didn't check failing checks too much, but I believe mini-css-extract-plugin does not support css-loader 7 yet.

@stof
Copy link
Member

stof commented Sep 4, 2024

Based on the documentation of mini-css-extract-plugin, its behavior depends on the modules.namedExport config of the css-loader, for which the default changed in v7. Maybe this is related to the issue.

@Kocal Kocal force-pushed the chore/css-loader-7 branch from e059cdb to fc93b00 Compare September 20, 2024 07:38
@stof
Copy link
Member

stof commented Sep 20, 2024

Looking at the test output, it seems related to the Vue integration.

@Kocal
Copy link
Member Author

Kocal commented Sep 20, 2024

Yeah... But, in fact, IINW we only test CSS modules with Vue, not React nor Svelte, so maybe it fails with those frameworks too.

For a moment I thought about automatically configure { namedExport: false, exportLocalsConvention: 'as-is' } if Encore.enableVueLoader() is called, but it's shaky IMO.

I suggest to always configure { namedExport: false, exportLocalsConvention: 'as-is' } and add some lines in the CHANGELOG about those changes. WDYT?

@Kocal
Copy link
Member Author

Kocal commented Sep 20, 2024

I've started to add tests about CSS Modules usages with other frameworks, coming in a new PR.

@Kocal Kocal mentioned this pull request Sep 20, 2024
Merged
Kocal added a commit that referenced this pull request Sep 20, 2024
@Kocal
minor #1343 Add tests for CSS Modules with React and Preact (Kocal)
675d68d 
This PR was squashed before being merged into the main branch.

Discussion
----------

Add tests for CSS Modules with React and Preact

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no <!-- please update CHANGELOG.md file -->
| Deprecations? | no <!-- please update CHANGELOG.md file -->
| Issues        | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exists, explain below instead -->
| License       | MIT

<!--
Replace this notice by a description of your feature/bugfix.
This will help reviewers and should be a good start for the documentation.

Additionally (see https://symfony.com/releases):
 - Always add tests and ensure they pass.
 - Features and deprecations must be submitted against the latest branch.
 - For new features, provide some code snippets to help understand usage.
 - Changelog entry should follow https://symfony.com/doc/current/contributing/code/conventions.html#writing-a-changelog-entry
 - Never break backward compatibility.
-->

Quick PR to add functional tests about using CSS Modules with React and Preact, to see it it also break CSS Modules with them in #1319.

Commits
-------

c22943b Add tests for CSS Modules with React
dbd5e5d Add tests for CSS Modules with Preact
@Kocal Kocal force-pushed the chore/css-loader-7 branch from fc93b00 to 8286aa1 Compare September 20, 2024 13:16
@Kocal
Copy link
Member Author

Kocal commented Sep 20, 2024

As expected, upgrading css-loader to v7 also break CSS modules for React and Preact integration.

Let's add { namedExport: false, exportLocalsConvention: 'as-is' } configuration by default?

@stof
Copy link
Member

stof commented Sep 20, 2024

I'm wondering whether we should change the config to use the old behavior or update our tests to follow the recommended usage of css-loader (and document the fact that css-loader 7 has a BC break when using CSS modules so they should either make their code compatible with the new version of configure things themselves).

Keeping a non-default configuration for css-loader forever looks weird to me. And shipping the BC break on a different schedule than the css-loader upgrade changing the default would make it harder to communicate about it.

@Kocal
Copy link
Member Author

Kocal commented Sep 20, 2024

When opening the PR, I started to update our tests and keep the breaking changes, but, IIRC CSS Modules were not usable with Vue Loader due to the way they are declared in .vue files.

I also prefer breaking things in a new major version, but we need to find a way for Vue support. I will restart checking about this in a few hours.

@stof
Copy link
Member

stof commented Sep 20, 2024

Looking at https://github.com/vuejs/vue-loader/blob/698636508e08f5379a57eaf086b5ff533af8e051/src/cssModules.ts#L9, it seems like vue-loader is not compatible with the new default of namedExport: true as it does not have the as *

@Kocal Kocal mentioned this pull request Sep 20, 2024
Open
@Kocal
Copy link
Member Author

Kocal commented Sep 20, 2024 •
edited
Loading

Someone opened a pull-request vuejs/vue-loader#1909 3 years ago, the author repeatedly rebased its PR, but it doesn't look like anyone cared about what was done. The last vue-loader release if from december 2023 too.

I want to upgrade to css-loader v7 for Encore v5, and I don't want the few users of Vue + CSS Modules to restrict us from upgrading (Pareto law).

What about:

  1. Upgrading css-loader v7
  2. Do not touch css-loader options
  3. In the CHANGELOG.md (I think I will write a UPGRADE.md file too), tells people about:
    • css-loader default options breaking changes, telling them to changes from import styles to import * as styles etc...
    • but, for Vue/CSS Modules users, tell them to configure the css-loader themselves through Encore.configureCssLoader()

WDYT?

@Kocal Kocal marked this pull request as ready for review September 20, 2024 17:56
@Kocal
[CSS Modules] Update our React/Preact tests to fit css-loader v7 defa…
ac0fd39 
…ult options, notify Vue users about the new behaviour and the solution
@Kocal Kocal force-pushed the chore/css-loader-7 branch from ed33ac2 to ac0fd39 Compare September 20, 2024 18:01
@Kocal
Copy link
Member Author

Kocal commented Sep 20, 2024 •
edited
Loading

I've just pushed my idea, I definitely think that's the best option for everyone.

@Kocal Kocal merged commit 524aa92 into symfony:main Sep 25, 2024
@Kocal Kocal deleted the chore/css-loader-7 branch September 25, 2024 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

5.0

Development

Successfully merging this pull request may close these issues.

2 participants

@Kocal @stof