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

fix: remove LightningCssMinimizerRspackPlugin exclude option default value #7811

Conversation

noshower
Copy link
Contributor

@noshower noshower commented Sep 5, 2024

Remove the default value of the lightningcss exclude option to make it consistent with the default of the lightningcss Playground.

In addition, if the default value is not removed, the CSS backward compatibility will fail. The transition from rspack.SwcCssMinimizerRspackPlugin to rspack.LightningCssMinimizerRspackPlugin will cause CSS compatibility issues.

For example, the original CSS code of top:0;right:0;bottom:0;left:0; becomes inset:0 after being compressed by LightningCssMinimizerRspackPlugin.

The inset property is a newer CSS property.


移除 lightningcss exclude 选项的默认值,使其与 lightningcss Playground 默认情况保持一致。

另外,不移除默认值,会导致 css 向下兼容失败,从 rspack.SwcCssMinimizerRspackPlugin 过渡到 rspack.LightningCssMinimizerRspackPlugin 会出现 css 兼容性问题。

例如,原先是 top:0;right:0;bottom:0;left:0; 的 css 代码,再经过 LightningCssMinimizerRspackPlugin 压缩后,变成 inset:0。

而 inset 属性是较新的 css 属性。

演示链接:https://lightningcss.dev/playground/index.html#%7B%22minify%22%3Afalse%2C%22customMedia%22%3Atrue%2C%22cssModules%22%3Afalse%2C%22analyzeDependencies%22%3Afalse%2C%22targets%22%3A%7B%22chrome%22%3A5046272%7D%2C%22include%22%3A0%2C%22exclude%22%3A1048575%2C%22source%22%3A%22%5Cn%5Cn.foo%20%7B%5Cn%20%20position%3Aabsolute%3B%5Cn%20%20top%3A0%3B%5Cn%20%20right%3A0%3B%5Cn%20%20left%3A0%3B%5Cn%20%20bottom%3A0%3B%5Cn%7D%5Cn%5Cn%5Cn%22%2C%22visitorEnabled%22%3Afalse%2C%22visitor%22%3A%22%7B%5Cn%20%20Color(color)%20%7B%5Cn%20%20%20%20if%20(color.type%20%3D%3D%3D%20'rgb')%20%7B%5Cn%20%20%20%20%20%20color.g%20%3D%200%3B%5Cn%20%20%20%20%20%20return%20color%3B%5Cn%20%20%20%20%7D%5Cn%20%20%7D%5Cn%7D%22%2C%22unusedSymbols%22%3A%5B%5D%2C%22version%22%3A%22local%22%7D

  • Tests updated (or not required).
  • Documentation updated (or not required).

@github-actions github-actions bot added the release: bug fix release: bug related release(mr only) label Sep 5, 2024
Copy link

netlify bot commented Sep 5, 2024

Deploy Preview for rspack canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit 3cb67f8
🔍 Latest deploy log https://app.netlify.com/sites/rspack/deploys/66e168f5673cea00089d33ce

@SoonIter
Copy link
Member

SoonIter commented Sep 5, 2024

related to parcel-bundler/lightningcss#792

@SoonIter SoonIter requested a review from chenjiahan September 5, 2024 12:13
@h-a-n-a
Copy link
Contributor

h-a-n-a commented Sep 5, 2024

Should check if this is considered as a breaking change.

@CLAassistant
Copy link

CLAassistant commented Sep 5, 2024

CLA assistant check
All committers have signed the CLA.

@noshower noshower force-pushed the fix/remove-lightningCssMinimizerRspackPlugin-options-exclude-default-value branch from cca58b2 to 255fcdb Compare September 5, 2024 12:56
@noshower
Copy link
Contributor Author

noshower commented Sep 5, 2024

Should check if this is considered as a breaking change.

首先可以确定的是,已经配置了 exclude 的,不会收到影响。

移除 exclude 默认配置后,压缩后的 css 兼容性将会与 targets 保持一致,兼容性更大。 除了产物变大,应该没有其他影响。

@chenjiahan
Copy link
Member

LightningCssMinimizerRspackPlugin adds the exclude option by default, because our goal is to avoid introducing unexpected CSS transformations in the minimizer.

Currently the exclude option does not work as expected, we should wait for the progress of parcel-bundler/lightningcss#792.

@noshower
Copy link
Contributor Author

noshower commented Sep 6, 2024

LightningCssMinimizerRspackPlugin adds the exclude option by default, because our goal is to avoid introducing unexpected CSS transformations in the minimizer.

Currently the exclude option does not work as expected, we should wait for the progress of parcel-bundler/lightningcss#792.

我觉得 lightningCss include 和 exclude 的行为没有问题。 大多数情况下,我们仅仅只需要设置 targets 就能满足我们使用。

include 和 exclude 是为了更细粒度的控制 lightningCss 的编译行为。

例如,用户的 targets 是 "chrome 120 ", 并使用了 css 嵌套语法,因为 chrome 120 支持嵌套语法,因此, 默认情况下 lightning Css 根据 targets 的配置不会编译 css 嵌套语法。 如果,用户不希望压缩后的代码中存在 css 嵌套语法,那么需要在 include 中包含 Nesting 。 示例: https://lightningcss.dev/playground/index.html#%7B%22minify%22%3Afalse%2C%22customMedia%22%3Atrue%2C%22cssModules%22%3Afalse%2C%22analyzeDependencies%22%3Afalse%2C%22targets%22%3A%7B%22chrome%22%3A7864320%7D%2C%22include%22%3A1%2C%22exclude%22%3A0%2C%22source%22%3A%22.a%20%7B%5Cn%20%20.b%20%7B%5Cn%20%20%20%20color%3A%20red%3B%5Cn%20%20%7D%5Cn%7D%5Cn%5Cn%22%2C%22visitorEnabled%22%3Afalse%2C%22visitor%22%3A%22%7B%5Cn%20%20Color(color)%20%7B%5Cn%20%20%20%20if%20(color.type%20%3D%3D%3D%20'rgb')%20%7B%5Cn%20%20%20%20%20%20color.g%20%3D%200%3B%5Cn%20%20%20%20%20%20return%20color%3B%5Cn%20%20%20%20%7D%5Cn%20%20%7D%5Cn%7D%22%2C%22unusedSymbols%22%3A%5B%5D%2C%22version%22%3A%22local%22%7D

例如,用户的 targets 是 ”chrome 89", 并使用了 css 嵌套语法,因为 chrome 89 不支持该语法,因此,默认情况下 lightning Css
根据 targets 的配置会将 嵌套语法编译成普通的写法。 如果,用户希望压缩后仍然使用 css 嵌套语法,那么需要在 exclude 中包含 Nesting 。示例:https://lightningcss.dev/playground/index.html#%7B%22minify%22%3Afalse%2C%22customMedia%22%3Atrue%2C%22cssModules%22%3Afalse%2C%22analyzeDependencies%22%3Afalse%2C%22targets%22%3A%7B%22chrome%22%3A5832704%7D%2C%22include%22%3A0%2C%22exclude%22%3A1%2C%22source%22%3A%22.a%20%7B%5Cn%20%20.b%20%7B%5Cn%20%20%20%20color%3A%20red%3B%5Cn%20%20%7D%5Cn%7D%5Cn%5Cn%22%2C%22visitorEnabled%22%3Afalse%2C%22visitor%22%3A%22%7B%5Cn%20%20Color(color)%20%7B%5Cn%20%20%20%20if%20(color.type%20%3D%3D%3D%20'rgb')%20%7B%5Cn%20%20%20%20%20%20color.g%20%3D%200%3B%5Cn%20%20%20%20%20%20return%20color%3B%5Cn%20%20%20%20%7D%5Cn%20%20%7D%5Cn%7D%22%2C%22unusedSymbols%22%3A%5B%5D%2C%22version%22%3A%22local%22%7D

总结

lightning css 压缩时会编译 css, 编译后的 css 满足 targets 配置的兼容性要求。

当用户期望部分 css 特性需要有更大的兼容性,可以使用 include 选项配置。
当用户期望部分 css 特性可以有更小的兼容性(例如,使用更简洁的 css 属性),可以使用 exclude 选项配置。

exclude 应该属于个性化选项,不该存在默认配置,否则会让配置了 targets 的用户,很难理解 lightningcssMinimizerRspackPlugin 的行为。让那些从 rspack.SwcCssMinimizerRspackPlugin 迁移到 rspack.LightningCssMinimizerRspackPlugin 的用户,产生潜在的线上问题。

@chenjiahan
Copy link
Member

Our goal is to ensure that the "LightningCssMinimizerRspackPlugin" does not transform CSS syntax but only performs CSS compression. This is the issue discussed in parcel-bundler/lightningcss#792.

We do not want to require users to configure a suitable target in order for the LightningCssMinimizerRspackPlugin to work correctly.

There are several reasons for this:

  • In webpack's API design, loaders are used for transformation, and minimizers are used for compression. If we also perform transformations in the minimizer, it would duplicate the transformation logic executed by lightningcss-loader, leading to potential behavior conflicts (e.g., if the configurations of the two are inconsistent).

  • LightningCssMinimizerRspackPlugin is a plugin that Rspack enables by default in production, while builtin:lightningcss-loader is an optional feature. This is similar to other bundlers in the community, where the bundler by default only performs CSS compression and does not perform CSS transformation.

  • If LightningCssMinimizerRspackPlugin performs transformations by default, users would need to manually configure the target to ensure that the build artifacts' size and compatibility meet the expected standards. This introduces additional configuration and understanding costs.


我们的目标是让「LightningCssMinimizerRspackPlugin」不 transform CSS 语法,只进行 CSS 压缩。这也是 parcel-bundler/lightningcss#792 里试图解决的问题。

我们不想要求用户必须配置一个合适的 target,才能使 LightningCssMinimizerRspackPlugin 正确工作。

原因有几点:

  • 在 webpack 的 API 设计里,loader 用于 transform,minimizer 用于压缩。如果我们在 minimizer 也做 transform,那么就与 lightningcss-loader 执行了重复的 transform 逻辑,存在潜在的行为冲突(例如两者配置不一致的情况)。

  • LightningCssMinimizerRspackPlugin 是 Rspack 在 production 默认启用的插件,而 builtin:lightningcss-loader 是可选的功能。这与社区其他 bundler 类似, bundler 默认只进行 CSS 压缩,不进行 CSS transform。

  • 如果 LightningCssMinimizerRspackPlugin 默认执行 transform,那么用户需要手动配置 target 才能使产物体积、产物兼容性达到预期的状态。这带来了额外的配置和理解成本。

@noshower
Copy link
Contributor Author

noshower commented Sep 6, 2024

Our goal is to ensure that the "LightningCssMinimizerRspackPlugin" does not transform CSS syntax but only performs CSS compression. This is the issue discussed in parcel-bundler/lightningcss#792.

We do not want to require users to configure a suitable target in order for the LightningCssMinimizerRspackPlugin to work correctly.

There are several reasons for this:

  • In webpack's API design, loaders are used for transformation, and minimizers are used for compression. If we also perform transformations in the minimizer, it would duplicate the transformation logic executed by lightningcss-loader, leading to potential behavior conflicts (e.g., if the configurations of the two are inconsistent).
  • LightningCssMinimizerRspackPlugin is a plugin that Rspack enables by default in production, while builtin:lightningcss-loader is an optional feature. This is similar to other bundlers in the community, where the bundler by default only performs CSS compression and does not perform CSS transformation.
  • If LightningCssMinimizerRspackPlugin performs transformations by default, users would need to manually configure the target to ensure that the build artifacts' size and compatibility meet the expected standards. This introduces additional configuration and understanding costs.

我们的目标是让「LightningCssMinimizerRspackPlugin」不 transform CSS 语法,只进行 CSS 压缩。这也是 parcel-bundler/lightningcss#792 里试图解决的问题。

我们不想要求用户必须配置一个合适的 target,才能使 LightningCssMinimizerRspackPlugin 正确工作。

原因有几点:

  • 在 webpack 的 API 设计里,loader 用于 transform,minimizer 用于压缩。如果我们在 minimizer 也做 transform,那么就与 lightningcss-loader 执行了重复的 transform 逻辑,存在潜在的行为冲突(例如两者配置不一致的情况)。
  • LightningCssMinimizerRspackPlugin 是 Rspack 在 production 默认启用的插件,而 builtin:lightningcss-loader 是可选的功能。这与社区其他 bundler 类似, bundler 默认只进行 CSS 压缩,不进行 CSS transform。
  • 如果 LightningCssMinimizerRspackPlugin 默认执行 transform,那么用户需要手动配置 target 才能使产物体积、产物兼容性达到预期的状态。这带来了额外的配置和理解成本。

实际上,使用 esbuild 进行压缩,默认情况下也会对 css 、js 语法进行调整。默认会转成更简洁的 js/css 语法。 这一点,至少 esbuild 文档中指出来了。 https://esbuild.github.io/api/#minify

另外,对于你所说的几点,可能都不会发生。原因如下:

  1. 很多前端项目应该都在使用 postcss-loader 对 css 进行降级处理
  2. 很多前端项目应该都配置了 browserslist. ,不需要手动指定 lightning css 的 targets
  3. 以上两点都有的项目,exclude 的默认值是 lightningcss 执行 transform 的罪魁祸首。也就是说,以上两点都有的项目,现在迁移到 LightningCssMinimizerRspackPlugin , 不配置 exclude ,很可能是存在 css 问题的, 原本经过 loader 处理,加了厂家前缀的 css, 在经过 lightning css 后全消失了。 演示链接 。 这是破坏性的问题,需要在迁移文档中明确指出

因此,不光是 exclude 需要删除默认值,targets 也需要删除默认值。

@chenjiahan
Copy link
Member

使用 esbuild 进行压缩,默认情况下也会对 css 、js 语法进行调整

esbuild minify 的时候调整语法是为了减小体积,不是为了语法降级。而且这个默认行为并不合适,我们内部有一些项目因为这个行为而遇到了兼容性问题。

很多前端项目应该都在使用 postcss-loader 对 css 进行降级处理

这一点跟我说的不冲突,正是因为 postcss-loader 已经做了降级,所以 LightningCssMinimizerRspackPlugin 不应该引入预期之外的语法版本调整。

很多前端项目应该都配置了 browserslist. ,不需要手动指定 lightning css 的 targets

LightningCssMinimizerRspackPlugin 并不会默认读取 browserslist,所以必须得手动指定。

以上两点都有的项目,exclude 的默认值是 lightningcss 执行 transform 的罪魁祸首。

这一点还不能确定是否为 LightningCSS 的 bug 还是 by design,所以我上面说了要等 Lightning CSS 给出回复。

@noshower
Copy link
Contributor Author

noshower commented Sep 6, 2024

使用 esbuild 进行压缩,默认情况下也会对 css 、js 语法进行调整

esbuild minify 的时候调整语法是为了减小体积,不是为了语法降级。而且这个默认行为并不合适,我们内部有一些项目因为这个行为而遇到了兼容性问题。

很多前端项目应该都在使用 postcss-loader 对 css 进行降级处理

这一点跟我说的不冲突,正是因为 postcss-loader 已经做了降级,所以 LightningCssMinimizerRspackPlugin 不应该引入预期之外的语法版本调整。

很多前端项目应该都配置了 browserslist. ,不需要手动指定 lightning css 的 targets

LightningCssMinimizerRspackPlugin 并不会默认读取 browserslist,所以必须得手动指定。

以上两点都有的项目,exclude 的默认值是 lightningcss 执行 transform 的罪魁祸首。

这一点还不能确定是否为 LightningCSS 的 bug 还是 by design,所以我上面说了要等 Lightning CSS 给出回复。

好的,另外,targets 的默认值是不是应该调整成 browserslist.

@chenjiahan
Copy link
Member

According to the discussion in parcel-bundler/lightningcss#792, Rspack will remove the default exclude and encourage users to always configure the correct target.

I think we can merge this PR, @ahabhgk what do you think?

@chenjiahan chenjiahan requested a review from ahabhgk September 11, 2024 05:39
@ahabhgk
Copy link
Contributor

ahabhgk commented Sep 11, 2024

Looks good to me, and since this will only downgrade syntax, instead of upgrading syntax, I think this is ok to consider as a non breaking change

@noshower
Copy link
Contributor Author

According to the discussion in parcel-bundler/lightningcss#792, Rspack will remove the default exclude and encourage users to always configure the correct target.

I think we can merge this PR, @ahabhgk what do you think?

如果同意此 PR,那么 target 的默认值,是否应该改成用户的 browerlist 配置。

@chenjiahan
Copy link
Member

I think this is ok to consider as a non breaking change

Agreed.

那么 target 的默认值,是否应该改成用户的 browerlist 配置。

I think this is reasonable.

@chenjiahan
Copy link
Member

LightningCssMinimizerRspackPlugin can set target in the following order:

  1. If target is configured, use it.
  2. Try to read browserslist as the default target.
  3. Use es6 as the default target if there is no browserslist file.

@chenjiahan
Copy link
Member

chenjiahan commented Sep 11, 2024

@noshower I think we can modify the default behavior of the target option in a separate PR. Can you update the test snapshot first to make this PR pass CI? This will allow the current PR to be merged.

@noshower
Copy link
Contributor Author

@noshower I think we can modify the default behavior of the target option in a separate PR. Can you update the test snapshot first to make this PR pass CI? This will allow the current PR to be merged.

我试试

@noshower noshower force-pushed the fix/remove-lightningCssMinimizerRspackPlugin-options-exclude-default-value branch from 57fa4e6 to 3cb67f8 Compare September 11, 2024 09:55
@chenjiahan chenjiahan enabled auto-merge (squash) September 11, 2024 10:00
@chenjiahan chenjiahan changed the title fix: remove LightningCssMinimizerRspackPlugin exclude option default … fix: remove LightningCssMinimizerRspackPlugin exclude option default value Sep 11, 2024
@chenjiahan chenjiahan merged commit b1bf153 into web-infra-dev:main Sep 11, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release: bug fix release: bug related release(mr only)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants