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

feat(webpack): set default sassImplementation to sass-embedded #30580

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Coly010
Copy link
Contributor

@Coly010 Coly010 commented Apr 2, 2025

Current Behavior

The sassImplementation option for Webpack and Rspack is set to sass by default.
sass-embedded offers a faster alternative.

Expected Behavior

Switch the default sassImplementation option to sass-embedded.

@Coly010 Coly010 requested a review from a team as a code owner April 2, 2025 09:03
@Coly010 Coly010 requested a review from leosvelperez April 2, 2025 09:03
@Coly010 Coly010 self-assigned this Apr 2, 2025
Copy link

vercel bot commented Apr 2, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Updated (UTC)
nx-dev ⬜️ Ignored (Inspect) Visit Preview Apr 2, 2025 9:05am

Copy link

nx-cloud bot commented Apr 2, 2025

View your CI Pipeline Execution ↗ for commit 44c4cdc.

Command Status Duration Result
nx affected --targets=lint,test,build,e2e,e2e-c... ✅ Succeeded 1m 18s View ↗
nx run-many -t check-imports check-commit check... ✅ Succeeded 20s View ↗
nx-cloud record -- nx-cloud conformance:check ✅ Succeeded 2s View ↗
nx-cloud record -- nx format:check --base=b3c6d... ✅ Succeeded 1s View ↗
nx-cloud record -- nx sync:check ✅ Succeeded <1s View ↗
nx documentation ✅ Succeeded <1s View ↗

☁️ Nx Cloud last updated this comment at 2025-04-02 09:53:04 UTC

@Coly010 Coly010 added target: next major version To be merged for the next major version release PR status: do not merge This will block a PR from being merged until this tag is removed. labels Apr 2, 2025
Copy link
Member

@leosvelperez leosvelperez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to call this out as a breaking change. Even though they are comparable functionality-wise, sass-embedded only works in selected platforms (the most used ones, but it could still break some setups).

@@ -161,7 +161,6 @@ export interface NxAppRspackPluginOptions {
* Add an additional chunk for the rspack runtime. Defaults to `true` when `target === 'web'`.
*/
runtimeChunk?: boolean;
// TODO(v21): Make Sass Embedded the default in version 21.
// TODO(v22): Remove in version 22.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the plan is to remove this option for v22, we should add a deprecation message.

@@ -221,7 +221,6 @@ export interface NxAppWebpackPluginOptions {
* Use tsconfig-paths-webpack-plugin to resolve modules using paths in the tsconfig file.
*/
useTsconfigPaths?: boolean;
// TODO(v21): Make Sass Embedded the default in version 21.
// TODO(v22): Remove in version 22.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here about the deprecation notice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR status: do not merge This will block a PR from being merged until this tag is removed. target: next major version To be merged for the next major version release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants