Make WordPress Core

Opened 17 months ago

Last modified 7 days ago

#63013 new enhancement

Twenty Nineteen: Use `sass` instead of `node-sass`

Reported by: debarghyabanerjee Owned by:
Priority: normal Milestone: Future Release
Component: Bundled Theme Version:
Severity: normal Keywords: has-patch
Cc: Focuses: javascript, css

Description

node-sass is deprecated and no longer maintained. We should switch to sass, the official and actively supported Sass compiler, to stay up-to-date with the latest features and improvements.

Change History (14)

This ticket was mentioned in PR #8404 on WordPress/wordpress-develop by @debarghyabanerjee.


17 months ago
#1

  • Keywords has-patch added; needs-patch removed

Trac Ticket: Core-63013

### Summary

This PR replaces node-sass with sass in the twentynineteen theme. node-sass is deprecated and no longer maintained, while sass is the official, actively supported implementation of Sass.

This ticket was mentioned in PR #8405 on WordPress/wordpress-develop by @debarghyabanerjee.


17 months ago
#2

Trac Ticket: Core-63013

### Summary

This PR replaces node-sass with sass in the twentynineteen theme. node-sass is deprecated and no longer maintained, while sass is the official, actively supported implementation of Sass.

#3 @sabernhardt
17 months ago

  • Component ThemesBundled Theme
  • Summary Update: Use `sass` instead of `node-sass` in twentynineteen themeTwenty Nineteen: Use `sass` instead of `node-sass`

@sainathpoojary also reported the deprecation on GitHub.

If the package switches to sass:

  • The output is very different, especially with the order of language-specific selectors. Setting up some linting rules could help (for example, with stylelint-config).
  • Please use --no-source-map. Twenty Nineteen does not have the .map files yet, and committing one change with the source maps could require refreshing any other pull requests that were made with older map files.

#4 follow-up: @debarghyabanerjee
17 months ago

  • Component Bundled ThemeThemes

Hi @sabernhardt,

  1. I have added the --no-source-map
  1. Do you suggest any specific linting rules that should be added in the stylelint-config?

#5 in reply to: ↑ 4 @sabernhardt
17 months ago

  • Component ThemesBundled Theme

Do you suggest any specific linting rules that should be added in the stylelint-config?

Unfortunately, I do not know linting rules well. Some of the rules for Twenty Twenty-One could also fit Twenty Nineteen.

Of course, my reason for mentioning stylelint-config is to avoid so many changes in the compiled output. A few of the differences should be good, but others surprised me.

  • The sass output ignores five of the selectors when it creates font overrides for each non-latin language: .not-found .page-title, .error-404 .page-title, input[type="button"], input[type="reset"], and input[type="submit"]. That was difficult to notice because the compiler rearranges the lists of language selectors.
  • The #005177 blue becomes rgb(0, 80.5, 119), slightly reducing its green value.
  • Many empty lines between rulesets are removed, but not all of them consistently.
  • Some comma-separated selectors share a line, where node-sass had given each its own line.

I should also point out that Dart Sass gives dozens of deprecation warnings when running the build, so switching to it would require additional changes soon.

This ticket was mentioned in PR #12156 on WordPress/wordpress-develop by @desrosj.


6 weeks ago
#6

This replaces node-sass with sass (Dart Sass).

Looking into the issues raised on the Trac ticket, here is some output from Claude around each one.

The sass output ignores five of the selectors when it creates font overrides for each non-latin language: .not-found .page-title, .error-404 .page-title, input[type="button"], input[type="reset"], and input[type="submit"]. That was difficult to notice because the compiler rearranges the lists of language selectors.

This appears to be caused by the redundancy elimination part of Dart Sass related to the use of @extend.

<details>
<summary>Full output from Claude: Output ignores five of the selectors when it creates font overrides for each non-latin language</summary>

How the SCSS builds the non-latin overrides:

  // sass/mixins/_mixins-master.scss
  %non-latin-fonts {
    @each $lang, $font__fallback in $font__fallbacks {
      &:lang(#{$lang}) { font-family: unquote($font__fallback); }
    }
  }

  @mixin font-family($font_family: $font__body) {
    font-family: $font_family;
    @extend %non-latin-fonts;
  }

  Every selector list that calls @include font-family(...) gets @extended into the placeholder, producing one giant
  <every-selector>:lang(ar) { ... } rule per language. LibSass emitted that list verbatim. Dart Sass runs a "redundancy
  elimination" pass on extend results — if one selector in the merged list is a structural superselector of another
  (matches a superset of elements), the more specific one is dropped.

  For the .page-title cases this is provably what happened. In sass/typography/_headings.scss:

  - Lines 11/33/75/144 include .not-found .page-title and .error-404 .page-title in lists that @include font-family(...).
  - Line 54: .page-title { @include font-family($font__body); } — bare .page-title is also extended.

  So .page-title:lang(ar) is already in the merged list, and Dart Sass treats .not-found .page-title:lang(ar) and
  .error-404 .page-title:lang(ar) as redundant subselectors and drops them.

  The three input[type=...] cases come from sass/forms/_buttons.scss:

  .button, button, input[type="button"], input[type="reset"], input[type="submit"] {
    @include font-family($font__heading);
  }

  The bare button element is in the same list. Dart Sass's selector-unification math considers
  input[type="button"]:lang(ar) covered by other selectors in the merged result and trims them too — button:lang(ar) is
  the most likely cause here as the trimmer can over-eagerly treat attribute-restricted versions of an element selector as
   redundant when both end up in the same combined list.

  This is a real change in matched elements:
  - .not-found .page-title:lang(ar) is fine — .page-title:lang(ar) does match it, so the font fallback still applies via
  the kept selector. (No visual regression.)
  - input[type="button"]:lang(ar) etc. — these are only covered if you accept that button:lang(ar) covers them, but a
  <input type="submit"> is not a <button> element. So Arabic/Hebrew/etc. submit buttons would lose the non-latin font
  fallback. That is a real visual regression.

  The fix is to refactor the mixin away from @extend. Either:

  @mixin font-family($font_family: $font__body) {
    font-family: $font_family;
    @each $lang, $font__fallback in $font__fallbacks {
      &:lang(#{$lang}) { font-family: unquote($font__fallback); }
    }
  }

  This emits the same rules inline at every call site (slightly more CSS in bytes, no extend trimming, output is stable).
  Or keep @extend but split the placeholder into per-language placeholders. The inline @each is simpler and idiomatic for
  current Sass.

</details>

The #005177 blue becomes rgb(0, 80.5, 119), slightly reducing its green value.

This one seems to be caused by lighten()/darken(), which have been deprecated. I've gone and hard coded the intended hex values to address this for now.

<details>
<summary>Full output from Claude: Why are hexidecimal colors output as rgb() and slightly different?</summary>

These come from the lighten()/darken() calls the deprecation warnings flagged in _comments.scss:240 and elsewhere. The

math:

  • $colorlink = #0073aa = rgb(0, 115, 170)
  • darken($colorlink, ~8%) round-trips through HSL.
  • LibSass clamped/rounded the resulting RGB channels to integers and emitted hex.
  • Dart Sass keeps fractional precision and emits rgb(0, 80.5, 119) rather than #005177.

Browsers accept fractional rgb() (CSS Color L4), so the rendered color is rgb(0, 80.5, 119) — half a step off the prior
rgb(0, 81, 119). Imperceptible visually, but technically not byte-identical and slightly off from what the designer
presumably intended (#005177).

The right fix is the same as resolving the deprecation: replace lighten()/darken() calls with the intended literal hex
(best — that's what the designer originally meant), or wrap with color.adjust() which is the modern API. As a stopgap
you can wrap with round() to coerce channels back to integers.

`
</details>

Many empty lines between rulesets are removed, but not all of them consistently.

While annoying, this one does not require any action. It's just a formatting issue.

From Claude:

Dart Sass's expanded formatter inserts blank lines around rule boundaries based on its own heuristics (e.g., always after @font-face, between top-level rules, but not consistently between sibling rules that came from a @for/@each loop). It does not preserve blank lines from your .scss source. There's no flag to make it match LibSass's "preserve source whitespace" behavior — this is just the formatter, and the inconsistency is baked in.

Some comma-separated selectors share a line, where node-sass had given each its own line.

Claude output:

As noted before, this is the @extend merge grouping. Dart Sass joins selectors that came from the same extend operation with , on a single line and uses ,\n between groups; LibSass put every selector on its own line unconditionally. No formatter flag.

After pushing a bit further, I had it create a small PostCSS plugin to enforce this.

Trac ticket: Core-63013.

## Use of AI Tools

AI assistance: Yes
Tool(s): Claude Code
Model(s): Opus 4.7
Used for: Researching the reasons behind the differences in built files after updating.

#7 @desrosj
6 weeks ago

  • Milestone Awaiting Review7.1

This appears to be a blocker for upgrading the version of Node.js from 20.x to 24.x in #65451.

I've created a new PR attempting to address some of the items called out by @sabernhardt above.

I have not yet reviewed the resulting changes past a cursory glance.

This ticket was mentioned in Slack in #core by adrianduffell. View the logs.


13 days ago

#9 @adrianduffell
13 days ago

This looks worth prioritizing to unblock Node 24. Does it need to be completed before the 7.1 beta? If so, it has a chance to be included if the PR can be reviewed in the coming days.

Last edited 13 days ago by adrianduffell (previous) (diff)

adrianduffell commented on PR #12156:


13 days ago
#10

@desrosj is this ready for review?

This ticket was mentioned in PR #12510 on WordPress/wordpress-develop by @sabernhardt.


9 days ago
#11

Co-authored by @desrosj

  • Replaces node-sass with sass in package, and updates the build scripts.
  • Replaces lighten(), darken(), and mix() with the previously computed color values.
  • Creates a new mixin for non-latin fonts so the Sass can still use @extend for 76 of the 81 selectors in style.css (and style-rtl.css) and 30 of 33 selectors in style-editor.css.
  • Adds the remaining selectors below their related rulesets with the new mixin.
  • Moves comments—or changes their syntax to //—so they do not print in an unrelated place, such as
    .main-navigation {
      /* Un-style buttons */
    }
    

Trac 63013

## Use of AI Tools

This is built on https://github.com/WordPress/wordpress-develop/pull/12156, which involved Claude and Copilot, but I did not use AI in addition to that.

@sabernhardt commented on PR #12510:


9 days ago
#12

GitHub Desktop told me the compiled stylesheets changed line endings to CRLF, but I saw them as LF in my file editor. I edited the .scss files in the browser, so they should have the correct line endings.

This ticket was mentioned in Slack in #core by adrianduffell. View the logs.


8 days ago

#14 @adrianduffell
7 days ago

  • Milestone 7.1Future Release

This was discussed in today's bug scrub. I was hoping it could make it in 7.1, but with the beta being released in the coming hours, we will have to punt. It would be good to pick up again in 7.2.

Note: See TracTickets for help on using tickets.