Update to modern Sass module system

noahtallen commented 2 years ago

TLDR: The base-styles package is incompatible with the new module system in Sass. We should update the package to be compatible with modern recommended Sass architecture.

What problem does this address?

Currently, the Gutenberg packages use the legacy module system with @import statements. @import is either currently or will soon be deprecated, and will be removed from sass within the next year or two. (More info in this blog post.) This Sass project has been ongoing since 2018, with the new module system launching in 2019. The new module system is currently recommended, and use of @import is discouraged. As a result, consumers of WordPress packages will increasingly try to use the new module system for Gutenberg packages. However, they will run into a few problems with the current architecture which prevent them from using the recommended sass syntax.

As an example, let's look at how one might use the default breakpoints and mixins for responsive styles with the legacy system, and then migrate it to the new system.

The current architecture

The current architecture of base-styles relies on variables being defined in the global scope. Mixins, for example, does not import the breakpoints itself. It relies on you to import the default breakpoints file or define the variables yourself.

// Current approach for including the default breakpoints. This effectively
// defines the variables inside breakpoints in the global scope.
@import '@wordpress/base-styles/breakpoints';

// One could even set the variables manually instead of importing them:
$break-small: 599px;

// Mixins now get used with the variables defined (or imported) above.
@import '@wordpress/base-styles/mixins';

.foo {
    @include break-small {
        margin: 8px;
    }
}

Migrating to `@use`

Now, let's migrate this usage to the new system! It's pretty simple with the sass migrator tool. @import is changed to @use, and you prefix namespaces to mixins, variables, etc you use.

@use '@wordpress/base-styles/breakpoints';
@use '@wordpress/base-styles/mixins';

.foo {
    @include mixins.break-small { // Note the "mixins" namespace
        margin: 8px;
    }
}

Errors after migration

However, this will fail with the following error:

ERROR in ./foo/style.scss
...
SassError: Undefined variable.
   ╷
39 │     @media (min-width: #{ ($break-small) }) {
   │                            ^^^^^^^^^^^^
   ╵
  ../../node_modules/@wordpress/base-styles/_mixins.scss 39:25                       break-small()
  ./foo/style.scss 5:3                                                               root stylesheet

The issue is that the new module system applies namespacing to imports. As a result, the $break-small variable from breakpoints is not accessible inside of mixins, since mixins doesn't import breakpoints. My first thought was to declare breakpoints on the global scope:

@use '@wordpress/base-styles/breakpoints' as *;
@use '@wordpress/base-styles/mixins' as *;

But this has the exact same problem. I'm guessing that the new module system is more strictly siloing the variables, so the global scope I've put breakpoints into in foo.scss is not available in mixins.

As a result, the base styles package is incompatible with the new module system. 3rd parties cannot update to the new module system until base-styles is compatible.

What is your proposed solution?

We need to migrate to the new module system. As a pre-req, we need to update Sass to something relatively recent, unless we've done that already. (And we need to use dart Sass instead of node-sass)

Fixing this example in base-styles

It's actually relatively easy to fix the example above:

First, I would update _breakpoints.scss to add !default at the end of every variable. This makes it possible to override them, which is a "feature" of the current global-scope architecture.

// _breakpoints.scss

// Just the break-small variable, as an example.
$break-small: 600px !default;

Second, I would update _mixins.scss to use the new module systems, and to explicitly import the default variables:

// Import default breakpoint variables:
@use './breakpoints';

// Works without further changes:
@use "./functions"; 

/**
 * Breakpoint mixins
 */

@mixin break-small() {
    // Access the variable on the breakpoints namespace:
    @media (min-width: #{ (breakpoints.$break-small) }) {
        @content;
    }
}

Trying our code example again

Now, let's try the example again:

@use '@wordpress/base-styles/breakpoints'; // this is now redundant and can be removed
@use '@wordpress/base-styles/mixins';

.foo {
    @include mixins.break-small { // Note the "mixins" namespace
        margin: 8px;
    }
}

This works successfully! There are two very nice things about this approach:

This migration can be preformed automatically with the migrator tool. The first line importing breakpoints is now unnecessary, but it can stick around without breaking anything after the migrator tool changes @import to @use.
Even though we changed everything to the new system under the hood, this file will still work with the previous syntax.

Bonus: overriding variables

One feature which changes is that variables have to be overridden with a different approach now. I think this syntax has been around for a while, but it's a better way to override things without relying on the global scope. You add with to include overrides at the end of an import statement. More here: https://sass-lang.com/documentation/at-rules/use#configuration

@use '@wordpress/base-styles/breakpoints' with(
    $break-small: 102px
);
@use '@wordpress/base-styles/mixins';

.foo {
    @include mixins.break-small {
        margin: 8px;
    }
}

Despite everything being namespaced correctly, the final CSS output will be what we expect:

@media (min-width: 102px) {
  .foo {
    margin: 8px;
  }
}

noahtallen commented 2 years ago

cc @jasmussen @gziolo for visibility -- would love some feedback! Or if you know anyone else who would be good to ping for feedback, please do so :)

Unless there are major concerns, I can look into creating a PR.

aristath commented 2 years ago

Maybe we should stop using Sass altogether? 😄 The main benefit of it when we started using it was the variables, and we can now achieve that using plain CSS. Mixins are nice but they are also confusing to the majority of people and we don't use them that often anyway. Our code could definitely be rewritten to use plain CSS. it may take a few more lines, but not that many. The benefit would be a lighter build process, fewer dependencies, and code more accessible to people that have not invested in learning Sass.

jasmussen commented 2 years ago

I think there's lots of opportunity for evolution here, especially in light of some of the CSS-in-JS changes going on with the component system. Notably private variables and the ability to nest rules, I feel, bring a great deal of coherence and readability to the codebase. While that's evolving, pragmatically and for the near term I'd think it worth it to keep sass around, and would love to help shepard any PRs along.

noahtallen commented 2 years ago

Maybe we should stop using Sass altogether?

Interesting thought! :D I won't speak towards usage in Gutenberg, but I know we import the base-styles a fair amount outside of Gutenberg at Automattic. (Examples: https://github.com/Automattic/wp-calypso/search?q=wordpress%2Fbase-styles) I think it has been useful for us as 3rd party consumers to be able to utilize the variables and mixins defined by Gutenberg as we make plugins and extensions for it.

I think that's where more updates to Sass could be useful. If we do want to continue exporting something for 3rd party consumers, IMO we should keep that up to date until we do decide to use a different solution (like all-in on CSS in JS or just plain CSS)

WordPress / gutenberg