Switch to sass-loader from node-sass

[noahtallen]

Changes proposed in this Pull Request

Since node-sass has been deprecated, we'll switch to sass (sometimes known as "dart sass"). Overall, this requires few changes. The only tricky part were the stylesheets we manually compiled. Those have been fixed with a simple bin script which does basically the same thing. The nice part is that it uses a custom importer (props to @jsnajdr), so we can remove tilde imports in the future. Comparing the stylesheets before and after and not seeing many changes.

Testing instructions

• Test suites should pass
• Smoke test calypso client
• Smoke test ETK

Related PRs

• Motivated by a bug in WIP: Dependency finder for smarter builds #52471
• Move away from tilde imports (Remove sass tilde imports #52800)
• fix some incorrect sass syntax (Fixup style syntax incompatible with dart-sass #52782)

[github-actions]

Test live: https://calypso.live/?branch=remove/sass-tilde-import

[scinos]

calypso-build did not use node-sass, so I removed it from its package.json.

How does that work? sass-loader docs says "sass-loader requires you to install either Dart Sass or Node Sass on your own".

Node-sass is deprecated, though. Should we replace it?

+1 to move to dart-sass.

[simison]

cc @Automattic/jetpack-crew same change might be possible in Jetpack.

[WunderBart]

The calypso client does not use sass-loader, but node-sass. Node-sass is deprecated, though. Should we replace it?

Looks like we'll need to replace it to make this one work? The build-css step doesn't seem to be able to resolve the new paths:

#15 1.747 $ run-p -s build-css:*
[12:13:14]#15 2.439 {
[12:13:14]#15 2.439   "status": 1,
[12:13:14]#15 2.439   "file": "/calypso/client/assets/stylesheets/shared/_typography.scss",
[12:13:14]#15 2.439   "line": 1,
[12:13:14]#15 2.439   "column": 1,
[12:13:14]#15 2.439   "message": "File to import not found or unreadable: @automattic/typography/styles/fonts.",
[12:13:14]#15 2.439   "formatted": "Error: File to import not found or unreadable: @automattic/typography/styles/fonts.\n        on line 1 of client/assets/stylesheets/shared/_typography.scss\n        from line 4 of client/assets/stylesheets/shared/_utils.scss\n        from line 4 of client/assets/stylesheets/reader-mobile.scss\n>> @import '@automattic/typography/styles/fonts';\n\n   ^\n"
[12:13:14]#15 2.439 }

[jeherve]

cc @Automattic/jetpack-crew same change might be possible in Jetpack.

Yes, that seems like a good idea. I believe we'll need to wait until Calypso Build is updated though, and then we'll need to complete the update in the repo.

Related PR: Automattic/jetpack#19374

[scinos]

The calypso client does not use sass-loader, but node-sass. Node-sass is deprecated, though. Should we replace it?

I believe calypso client uses both:

• node-sass as part of some scripts, as @WunderBart pointed out.
• sass-loader in client/webpack.config.js, via require( '@automattic/calypso-build/webpack/sass' )

[noahtallen]

How does that work? sass-loader docs says "sass-loader requires you to install either Dart Sass or Node Sass on your own".

I assume via webpack somehow (possibly MiniCssExtractPlugin?)

Basically, node-sass is never referenced under the packages/calypso-build directory. Maybe it's needed as a peer dependency?

[noahtallen]

It looks like the version on npm is just pure JS. There is a faster dart implementation, but it requires installing the dart SDK and dart packages. I assume we want to use the npm version since supporting the Dart toolchain would be tricky.

[scinos]

Basically, node-sass is never referenced under the packages/calypso-build directory. Maybe it's needed as a peer dependency?

sass-loader does use node-sass in https://github.com/webpack-contrib/sass-loader/blob/5ec86a528d36ef0610cc4f09ed11b8675a722678/src/utils.js#L7, however it doesn't declare it as a peerDependency.

I think we need to bring back the node-sass (or sass) dependency for calypso-build. Otherwise this works by accident, because some other package puts node-sass into ./node_modules/.

I assume we want to use the npm version since supporting the Dart toolchain would be tricky.

It would be nice to know profile sass vs node-sass to know how slower it is.

[jsnajdr]

Some review notes:
In addition to building styles with webpack and sass-loader, we also build some styles by directly calling node-sass in the build-css task in package.json. And because the node_modules resolving is a feature of sass-loader, not node-sass or dart-sass, it doesn't work in build-css.

We might want to migrate from node-sass to sass (dart-sass) separately.

Compiling our styles with dart-sass shows some syntax errors that dart-sass is sensitive about. I fixed them in #52782 which can be merged separately.

I get this error when trying yarn build-client-evergreen:

ERROR in ./assets/stylesheets/style.scss
Module build failed (from ../node_modules/mini-css-extract-plugin/dist/loader.js):
ModuleNotFoundError: Module not found: Error: Can't resolve './@wordpress/components/build-style/style.css' in '/Users/jsnajdr/src/wp-calypso/client/assets/stylesheets'

Apparently the new sass-loader can't resolve

@import '~@wordpress/components/build-style/style.css';

for some reason -- maybe the .css suffix?

[noahtallen]

Apparently the new sass-loader can't resolve

Hm, good catch. I'll look into that.

Edit, it looks like you're right. It compiles in this format: @import '@wordpress/components/build-style/style';

We might want to migrate from node-sass to sass (dart-sass) separately.

This is a good idea. I think I'll refactor this PR into switching to dart-sass, and move the import changes into another PR

[matticbot]

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.

[jsnajdr]

The build-css tasks need to be reworked:

• to use a node_modules-resolving importer. The dart-sass JS interface (render, renderSync) supports an importer option, but the CLI doesn't. We'll need to write a custom JS script for compiling these stylesheets.
• dart-sass also doesn't support the --output-style compressed option. We'll need to use cssnano as a part ot the postcss postprocessing pipeline. Need to ensure that postcss inside webpack build doesn't minify, while postcss in build-css does minify.

[fullofcaffeine]

I haven't bench-marked, but by doing some shallow research, It looks like the JS version of dart-sass (the one that doesn't run in the DartVM) is slower than node-sass. Are we taking that into account?

[jsnajdr]

There are other reasons why folks prefer dart-sass over node-sass:

• node-sass is deprecated since Oct 2020 and only dart-sass continues to be developed.
• node-sass package contents is a empty CLI shell and the real libsass is a native library that needs to be dowloaded in a postinstall script. That download is unreliable. Folks regularly end up with broken node-sass and need help. And postinstall scripts are inherently insecure (they can do anything) and many security-conscious people disable them globally.

The performance story that @fullofcaffeine shares raises some questions. The stats they share are for a very short compilation, where most of the time is likely spent just booting up Node.js.

What would be the results for, e.g., a webpack compilation where all SCSS processing is done in a warmed-up VM with the critical code JIT-ted? The author says: "I had a project with +20 themes, it took 30 seconds with node-sass, but I tried to use Dart-Sass(JS), and it took a century!" Century is certainly an exaggeration -- it would be nice to know more precise results 🙂

Why would a Dart VM be orders of magnitude faster than a V8 JS VM? That sounds implausible. V8 is supposed to be monstrously optimized.

[noahtallen]

Some anecdotal data we have is that Tumblr folks recently switched to dart sass and did not notice worse performance. I hope to do some basic benchmarking before merging as well.

[noahtallen]

I did a quick look into using webpack (I still think that would be better for long-term maintainability), but it seemed a bit more difficult. So I just added a simple bin script which uses the same package importer we previously used. The PostCSS processing is unchanged, since we can compress directly with sass.

[jsnajdr]

I did a quick look into using webpack (I still think that would be better for long-term maintainability), but it seemed a bit more difficult.

Webpack is really meant to bundling JS modules, and CSS is more of an afterthought, where all the extract plugins and CSS loaders are quite complicated. I was able to come up with the following script that uses the webpack's enhanced-resolve package to implement a custom resolver:

#!/usr/bin/env node

const sass = require( 'sass' );
const fs = require( 'fs' );
const resolve = require( 'enhanced-resolve' );
const yargs = require( 'yargs' );

// parse CLI, require --in and --out args
const args = yargs
	.usage( 'Usage: $0' )
	.option( 'in', { describe: 'Input file' } )
	.option( 'out', { describe: 'Output file' } )
	.demandOption( [ 'in', 'out' ] ).argv;

// create a webpack resolver that finds SCSS files. Inspired by `sass-loader` resolver.
const resolver = resolve.create( {
	conditionNames: [ 'sass', 'style' ],
	mainFields: [ 'sass', 'style', 'main' ],
	mainFiles: [ '_index', 'index' ],
	extensions: [ '.sass', '.scss', '.css' ],
	restrictions: [ /\.((sa|sc|c)ss)$/i ],
	preferRelative: true,
} );

// `dart-sass` custom importer
const importer = ( url, prev, done ) => {
	// strip the leading tilde
	url = url.replace( /^~/, '' );
	resolver( prev, url, ( error, result ) => {
		if ( error ) {
			// if webpack can't resolve the module, let further importers resolve the original URL
			done( { file: url } );
		} else {
			// resolve with the webpack result
			done( { file: result } );
		}
	} );
};

sass.render( { file: args.in, importer, outputStyle: 'compressed' }, ( err, output ) => {
	if ( err ) {
		console.error( 'error', err );
		return;
	}
	fs.writeFileSync( args.out, output.css );
} );

It's very similar to your version that uses node-sass-package-importer. The resolve.create configuration is copied from sass-loader. When encountering an import like:

@import 'calypso/style';

It first tries to find a relative ./calypso/style.scss file (or ./calypso/style/index.scss etc) and when it doesn't find that, does a node_modules lookup for a calypso package.

[jsnajdr]

We keep the importer, since the same API is shared with dart sass.

I experimented with node-sass-package-importer for a while, and found out it requires the package imports to be prefixed with ~. It's not compatible with the plan is to remove the tilde prefixes, after the latest sass-loader stopped requiring them.

[noahtallen]

Webpack is really meant to bundling JS modules, and CSS is more of an afterthought, where all the extract plugins and CSS loaders are quite complicated.

Makes sense. I was hoping that there might be a way to just render a stylesheet using the existing calypso-build toolchain, since it has all those plugins ready to go already.

I was able to come up with the following script that uses the webpack's enhanced-resolve package to implement a custom resolver

Very nice! I've just committed those changes to the branch. I really appreciate the time you spent on that. Using enhanced-resolve is a good idea -- on my first pass, I wrote a quick-and-dirty resolver before trying the node-sass importer. This is definitely a lot better 👍

I experimented with node-sass-package-importer for a while, and found out it requires the package imports to be prefixed with ~. It's not compatible with the plan is to remove the tilde prefixes, after the latest sass-loader stopped requiring them.

:/ That's really unfortunate. I was worried that might be the case, but hadn't tested yet. Good catch!

With that, I've pushed your changes to the branch, so I think it should be ready for another look.

[scinos]

Out of curiosity (and definitely not to be fixed in this PR): why we need a special command to generate 2 specific CSS files? Why can't they be part of an entrypoint, and let webpack generate them like it generates hundreds of public/evergreen/*.css files?

[jsnajdr]

Why can't they be part of an entrypoint, and let webpack generate them like it generates hundreds of public/evergreen/*.css files?

They are not part of the Calypso app at all. The Calypso deployment process only builds them from SCSS sources and deploys them so they are accessible at https://wordpress.com/calypso/directly.css and https://wordpress.com/calypso/reader-mobile.css URLs. Then they are loaded by third party pages: a custom chat widget at directly.com, and the WordPress mobile app's Reader HTML view.

That places the following constraints on building them with webpack:

• the entrypoints don't have any useful JS asset. There are JS files with css-loader runtime bits in them, all ultimately resolving to noop. It would be nice to tell webpack to skip emitting these assets.
• the file names need to be strictly directly.css, with no variable hashes etc. And the output path is public/, with no evergreen or fallback subdirectories.
• the directly entrypoint needs to be prohibited from sharing any chunks. Otherwise, nothing prevents webpack from putting part of the CSS into directly.css and another part into some shared CSS chunk file. A consumer that uses the webpack chunk manifest would know it needs to load multiple CSS files for the directly entrypoint, but we are just exposing a static URL here.
• the chunk manifest that the web app downloads as part of the webpack runtime should not contain info about the directly and reader-mobile entrypoints, chunks and their assets. They will never be used by the app.

If the above can be achieved with webpack, and if it's not in the end more complex than the render-stylesheet script, let's do it.

[jsnajdr]

I like this and it works well. I verified that the directly.css file hasn't changed in any material way.

There might be a pre-existing bug though: the directly.css and reader-mobile.css files contain references to variables like var( --color-primary ) without these variables being defined anywhere in the stylesheet.

@leandroalonso @bluefuton Is that OK for the reader-mobile.css stylesheet? Aren't there broken styles/missing colors in the mobile app's Reader view?

I suspect it's been broken in Aug 2020 by a PR that disabled the postcss-custom-properties plugin unless it's explicitly opted-into: #42829.

[scinos]

If the above can be achieved with webpack, and if it's not in the end more complex than the render-stylesheet script, let's do it.

Thanks for the explanation, it makes sense.

The first idea that comes to mind after reading the constraints would be to create their own "app" (eg: app/external-css) with its own (and very simple) webpack.config.js. I'll explore that idea if I have some spare time.

[leandroalonso]

@jsnajdr

@leandroalonso @bluefuton Is that OK for the reader-mobile.css stylesheet? Aren't there broken styles/missing colors in the mobile app's Reader view?

Yes, those colors are defined in the app itself. Eg.: https://github.com/wordpress-mobile/WordPress-iOS/blob/develop/WordPress/Classes/ViewRelated/Reader/Detail/WebView/ReaderWebView.swift#L206