feat(material/core): migrate to the Sass module system #21204

crisbeto · 2020-12-03T06:41:21Z

Migrates all of our existing code to the Sass module system in a backwards-compatible way. These changes are split up into 3 commits to make them easier to review:

Has all the changes necessary for the migration itself, including:

The migration script itself which is intended to be a one-off script, but I'm including it here so we can review the steps that I used to do the migration. I can remove it, if we don't think it's useful.
Some adjustments to the MDC form field theme which the migrator had a hard time dealing with.
A workaround in the core/theming/_theming.scss file which will be reverted in the third commit, after the migration is finished. This is another case the automated migrator was struggling with.
Using Sass modules to create the theming bundle and removing the dependency on scss-bundle.
Fixing a transient dependency on @types/sass. This used to come from scss-bundle, but we need it explicitly after the dependency was removed.

The actual migration. All changes in this commit have been made by the migration script.
Post-migration cleanup, including:

Removing the workaround from core/theming/_theming.scss in the first commit.
Manually fixing all of the Stylelint failures that couldn't be fixed automatically.
Updating one of our custom lint rules to account for the new syntax.

Note that these changes didn't migrate our imports from @material/*, because the migration script doesn't work with styles coming from the node_modules. It supports imports starting with ~, but they only work for the first level and break down if there's another absolute import within the package. I will make another manual pass over the remaining @import usages once these changes have landed.

jelbourn

I didn't finish looking through everything but I think I looked at the most important bits. Going along with this PR, it would be nice to also reference that changes we'll need for the theming docs and for the docs site. We'll probably have to make sure to update aio as well.

scripts/migrate-sass-modules.js

jelbourn · 2020-12-08T00:45:19Z

src/cdk/overlay/_overlay.import.scss

@@ -0,0 +1,8 @@
+@forward 'overlay' hide $dark-backdrop-background, $z-index-overlay, $z-index-overlay-backdrop,


For the cdk entry points with Sass (a11y, overlay, textfield), I think we need to also include these .import.scss files in the root of the cdk package. We currently copy each partial to the root, so we need to keep backwards compatibility there.

Separately, I think we should introduce a _cdk.scss file in the root of the package that should become the primary entry point into cdk styles to let people do stuff like

@use '~@angular/cdk'; @include cdk.a11y(); @include cdk.overlay();

I'm not sure I get this part:

We currently copy each partial to the root, so we need to keep backwards compatibility there.

My understanding is that all the Sass files get copied to the same place in the dist so as long as the .import file is next to the base .scss file, everything should work like before. Here's what the release output for cdk/overlay looks like:

As for the proposal to add a cdk.scss, I agree but I'd rather do it in a separate PR so this one only has the migration-specific changes.

I just tried this locally and everything does work as expected, I just don't understand why it works. We copy _a11y.scss, _overlay.scss, and _text-field.scss into the root of the package. Since the names of the mixins changed (e.g. cdk-overlay to overlay), I thought that the following would fail:

@import '~@angular/cdk/overlay'; @include cdk-overlay();

But it actually works just fine.

That doesn't sound right, you might have something cached which is preventing it from failing. I'll look into copying the .import files too.

jelbourn · 2020-12-08T00:46:47Z

src/cdk/a11y/_a11y.scss

@@ -1,4 +1,4 @@
-@mixin cdk-a11y {
+@mixin a11y {


This could be in a follow-up PR, but I'd like to rename this mixin to something like visually-hidden since my original name here (cdk-a11y) was really way too vague (I had thought we'd add more stuff to it, but that ended up not happening).

(with the old name deprecated for backwards compatibility, of course)

It can be done in a follow-up. I'm trying to keep this PR only to the migration itself.

jelbourn · 2020-12-08T01:37:46Z

src/cdk/a11y/_a11y.import.scss

@@ -0,0 +1,2 @@
+@forward 'a11y' hide a11y, high-contrast;


What happens if we remove this top line? My guess as to what's happening here is that it's attempting to forward cdk-optionally-nest-content, but that then gets hidden on the following line.

If we removed this line, somebody using @import will get global mixins called a11y and high-contrast.

I tried something like this locally:

// parent.scss @mixin one() { .one { color: red; } } @mixin two() { .two { background: blue; } } @mixin three() { .three { outline: yellow;} }

// child.scss @forward 'parent' as num-* hide three;

// leaf.scss @import 'child'; @include one();

Running sass ./leaf.scss errors here with Error: Undefined mixin. since the mixin is aliased as part of the @forward. Including num-one() instead works fine. I think that without explicitly forwarding the mixins, they aren't surfaced?

src/cdk/a11y/_a11y.import.scss

jelbourn · 2020-12-08T01:43:28Z

src/material/_theming.scss

@@ -0,0 +1,8 @@
+// Forwards all public API mixins so they can be imported from a single entry point.


Do we need a theming.import.scss in the root?

I'm not sure. This file is primarily tailored towards @import users since all the symbols will be prefixed with mat-. I don't know whether we'd want something like this for @use users, because there's a high chance for collisions (e.g. all themes have a mixin called color). It might be better to recommend using deep imports together with @use.

We can probably tackle this in a follow-up. I think for @use API, we'd want to have e.g.

@use '~@angular/material' as mat; @include mat.button-theme(...);

whereas for @import we'd obviously need to keep the compat

jelbourn · 2020-12-08T01:44:21Z

src/material/_theming.scss

+@forward './core/color/all-color.import';
+@forward './core/density/private/all-density.import';
+@forward './core/theming/all-theme.import';
+@forward './core/typography/all-typography.import';


Potentially for a follow up: it would be nice to have some sort of public api gold for Sass, since it's hard to tell from here all the stuff this is transitively forwarding.

I suspect that this will be a non-trivial amount of work since the PostCSS AST which we're using for Sass doesn't have the same APIs as the ones we use to generate the TS goldens. The Sass compiler probably has a better AST, but I don't know if it's public.

crisbeto

@jelbourn all of the feedback should have been addressed.

As for updating docs and examples, everything is still backwards-compatible so I'd rather land only the migration changes first and then everything can be adjusted. There's a bit more work left to migrate the @material/* imports, but that has to be done by hand, because the migration script wasn't able to do it.

crisbeto · 2020-12-08T19:41:30Z

src/cdk/a11y/_a11y.import.scss

@@ -0,0 +1,2 @@
+@forward 'a11y' hide a11y, high-contrast;


If we removed this line, somebody using @import will get global mixins called a11y and high-contrast.

src/cdk/a11y/_a11y.import.scss

crisbeto · 2020-12-08T19:42:09Z

src/cdk/a11y/_a11y.scss

@@ -1,4 +1,4 @@
-@mixin cdk-a11y {
+@mixin a11y {


It can be done in a follow-up. I'm trying to keep this PR only to the migration itself.

crisbeto · 2020-12-08T19:59:26Z

src/cdk/overlay/_overlay.import.scss

@@ -0,0 +1,8 @@
+@forward 'overlay' hide $dark-backdrop-background, $z-index-overlay, $z-index-overlay-backdrop,


I'm not sure I get this part:

We currently copy each partial to the root, so we need to keep backwards compatibility there.

My understanding is that all the Sass files get copied to the same place in the dist so as long as the .import file is next to the base .scss file, everything should work like before. Here's what the release output for cdk/overlay looks like:

As for the proposal to add a cdk.scss, I agree but I'd rather do it in a separate PR so this one only has the migration-specific changes.

jelbourn

LGTM

Caretaker note: Please let me know if this PR has significant failures and we can look at it together

jelbourn · 2020-12-08T23:47:54Z

Also meant to add: definitely some space for tweaking things, but +1 to getting this in and iterating in follow-ups

jelbourn · 2020-12-09T00:34:56Z

@crisbeto so, I did some preliminary testing before running a google presubmit, and discovered that there are about 200 files in google already import our stuff with @use. This means folks have stuff like

@use 'path/to/angular_material/src/blah/theming';

.my-rule {
  color: theming.mat-color($palette, 500);
}

So this change breaks all those apps since we renamed the mixins/functions. This made me realize that for anyone using @use already (internally and externally), this would be a breaking change. So I think we have to add @forward aliases for all the mixins and functions with their original prefixes and then deprecate those APIs, removing them in a later version. This also means we probably need to write a migrator for folks, since we're the ones making a breaking API change.

Splaktar · 2021-02-22T18:47:31Z

🎉

* In angular#21204 the `private-` prefix was unintentionally removed from all of the symbols. These changes restore the prefix. * Fixes the prefixes on the expansion panel theming mixins.

* In #21204 the `private-` prefix was unintentionally removed from all of the symbols. These changes restore the prefix. * Fixes the prefixes on the expansion panel theming mixins.

angular-automatic-lock-bot · 2021-03-25T00:06:02Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 3, 2020

crisbeto force-pushed the sass-modules branch 3 times, most recently from 4484711 to 4c3141c Compare December 3, 2020 19:17

crisbeto closed this Dec 3, 2020

crisbeto reopened this Dec 3, 2020

crisbeto force-pushed the sass-modules branch 4 times, most recently from e3d89a2 to d6d5433 Compare December 3, 2020 20:16

crisbeto marked this pull request as ready for review December 3, 2020 20:30

crisbeto requested review from andrewseguin, devversion, jelbourn, mmalerba and a team as code owners December 3, 2020 20:30

crisbeto added P2 The issue is important to a large percentage of users, with a workaround target: minor This PR is targeted for the next minor release labels Dec 3, 2020

crisbeto assigned jelbourn, mmalerba, annieyw and wagnermaciel Dec 3, 2020

crisbeto force-pushed the sass-modules branch from d6d5433 to a1ca67c Compare December 5, 2020 15:57

jelbourn reviewed Dec 8, 2020

View reviewed changes

crisbeto commented Dec 8, 2020

View reviewed changes

crisbeto force-pushed the sass-modules branch from a1ca67c to 628f41a Compare December 8, 2020 20:02

jelbourn approved these changes Dec 8, 2020

View reviewed changes

jelbourn added the action: merge The PR is ready for merge by the caretaker label Dec 8, 2020

crisbeto force-pushed the sass-modules branch 7 times, most recently from 6ec3f26 to 4d57754 Compare February 19, 2021 20:40

crisbeto added 2 commits February 20, 2021 09:12

refactor(material/core): sass module migration preparation

84f273f

refactor(material/core): run sass module migration

d94af9c

crisbeto force-pushed the sass-modules branch from 4d57754 to 95c58d0 Compare February 20, 2021 08:15

refactor(material/core): post migration fixup

d43cb99

crisbeto force-pushed the sass-modules branch from 95c58d0 to d43cb99 Compare February 20, 2021 08:23

crisbeto added target: major This PR is targeted for the next major release and removed target: minor This PR is targeted for the next minor release labels Feb 20, 2021

andrewseguin merged commit 53e98b1 into angular:master Feb 22, 2021

crisbeto mentioned this pull request Feb 23, 2021

fix(material/core): restore private- mixin prefix #21989

Merged

angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(material/core): migrate to the Sass module system #21204

feat(material/core): migrate to the Sass module system #21204

crisbeto commented Dec 3, 2020

jelbourn left a comment

jelbourn Dec 8, 2020

crisbeto Dec 8, 2020

jelbourn Dec 8, 2020

crisbeto Dec 9, 2020

jelbourn Dec 8, 2020

crisbeto Dec 8, 2020

jelbourn Dec 8, 2020

crisbeto Dec 8, 2020

jelbourn Dec 8, 2020

jelbourn Dec 8, 2020

crisbeto Dec 8, 2020 •

edited

Loading

jelbourn Dec 8, 2020

jelbourn Dec 8, 2020

crisbeto Dec 8, 2020

crisbeto left a comment

crisbeto Dec 8, 2020

crisbeto Dec 8, 2020

crisbeto Dec 8, 2020

jelbourn left a comment

jelbourn commented Dec 8, 2020

jelbourn commented Dec 9, 2020 •

edited

Loading

Splaktar commented Feb 22, 2021

angular-automatic-lock-bot bot commented Mar 25, 2021

		@@ -0,0 +1,8 @@
		@forward 'overlay' hide $dark-backdrop-background, $z-index-overlay, $z-index-overlay-backdrop,

		@@ -0,0 +1,8 @@
		// Forwards all public API mixins so they can be imported from a single entry point.

feat(material/core): migrate to the Sass module system #21204

feat(material/core): migrate to the Sass module system #21204

Conversation

crisbeto commented Dec 3, 2020

jelbourn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto Dec 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelbourn left a comment

Choose a reason for hiding this comment

jelbourn commented Dec 8, 2020

jelbourn commented Dec 9, 2020 • edited Loading

Splaktar commented Feb 22, 2021

angular-automatic-lock-bot bot commented Mar 25, 2021

crisbeto Dec 8, 2020 •

edited

Loading

jelbourn commented Dec 9, 2020 •

edited

Loading