Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Sep 25, 2024

Conversation

Kocal
Copy link
Member

@Kocal Kocal commented Sep 1, 2024

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

@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 added a commit that referenced this pull request Sep 20, 2024
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
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
Copy link
Member Author

Kocal commented Sep 20, 2024

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
…ult options, notify Vue users about the new behaviour and the solution
@Kocal
Copy link
Member Author

Kocal commented Sep 20, 2024

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
28 checks passed
@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants