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

Deps: Use sass-loader v13 #99412

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from
Open

Deps: Use sass-loader v13 #99412

wants to merge 2 commits into from

Conversation

tyxla
Copy link
Member

@tyxla tyxla commented Feb 6, 2025

Proposed Changes

Consistently use sass-loader@13 instead of 3 different versions.

Why are these changes being made?

During the latest WordPress monorepo upgrade (#96470) I noticed this inconsistency and thought it might make sense to be fixed.

Testing Instructions

  • Build should work well and all checks should be green.
  • Do some smoke testing and verify things work well.

@tyxla tyxla self-assigned this Feb 6, 2025
@matticbot
Copy link
Contributor

matticbot commented Feb 6, 2025

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

  • blaze-dashboard
  • command-palette-wp-admin
  • help-center
  • newsletter
  • notifications
  • odyssey-stats
  • whats-new
  • wpcom-block-editor

To test WordPress.com changes, run install-plugin.sh $pluginSlug update/sass-loader-13 on your sandbox.

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@tyxla tyxla force-pushed the update/sass-loader-13 branch from 31123c9 to 41be4eb Compare February 13, 2025 13:10
@tyxla tyxla added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. dependencies Pull requests that update a dependency file labels Feb 13, 2025
@tyxla tyxla requested a review from a team February 13, 2025 13:12
@tyxla tyxla marked this pull request as ready for review February 13, 2025 13:12
Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

Looking at the CHANGELOG, the only 2 breaking changes possible affecting us may be:

Should we address / apply any refactors before updating?

@m1r0
Copy link
Member

m1r0 commented Feb 18, 2025

Should we address / apply any refactors before updating?

I'm on a dependency update rotation so I can take a look. Is that ok with you, @tyxla?

@tyxla
Copy link
Member Author

tyxla commented Feb 18, 2025

@m1r0 definitely! I haven't had time to look into it today, so feel free to take over!

@m1r0 m1r0 force-pushed the update/sass-loader-13 branch from 41be4eb to 8400d2f Compare February 20, 2025 13:41
@m1r0 m1r0 requested a review from a team as a code owner February 20, 2025 16:14
@m1r0
Copy link
Member

m1r0 commented Feb 21, 2025

emit @warn at-rules as webpack warnings by default, if you want to revert behavior please use the warnRuleAsWarning option (webpack-contrib/sass-loader#1054) (58ffb68)

I was not able to get warnRuleAsWarning to work. I've tried adding the property to the sass-loader options but there's no difference if it's true or false - the warnings are not displayed in the console when running yarn start. On trunk, when using @warn, I can see the warnings in the console. Any ideas? 🤔

Trunk:
image

Now:
image

@tyxla
Copy link
Member Author

tyxla commented Feb 24, 2025

@m1r0 can you please provide a diff to demonstrate what you tried? Thanks!

@m1r0
Copy link
Member

m1r0 commented Feb 24, 2025

Sure, here you go:

diff --git a/client/landing/stepper/declarative-flow/internals/global.scss b/client/landing/stepper/declarative-flow/internals/global.scss
index cde9714f3ef..f31e8304e65 100644
--- a/client/landing/stepper/declarative-flow/internals/global.scss
+++ b/client/landing/stepper/declarative-flow/internals/global.scss
@@ -12,6 +12,8 @@
  */
 @import "calypso/blocks/import/style/base";

+@warn "THIS IS A TEST WARNING";
+
 /**
  * General onboarding styling
  */
diff --git a/packages/calypso-build/webpack/sass.js b/packages/calypso-build/webpack/sass.js
index 09c12a634e5..95e501938fe 100644
--- a/packages/calypso-build/webpack/sass.js
+++ b/packages/calypso-build/webpack/sass.js
@@ -36,6 +36,7 @@ module.exports.loader = ( { includePaths, prelude, postCssOptions } ) => ( {
 		{
 			loader: require.resolve( 'sass-loader' ),
 			options: {
+				warnRuleAsWarning: true,
 				additionalData: prelude,
 				sassOptions: {
 					includePaths,

Note that warnRuleAsWarning defaults to false on v13.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants