Skip to content
This repository has been archived by the owner on Sep 5, 2024. It is now read-only.

build: fix SCSS division deprecation #12112

Merged
merged 3 commits into from
Nov 30, 2021
Merged

Conversation

superheri
Copy link
Contributor

@superheri superheri commented Jul 5, 2021

AngularJS Material is in LTS mode

We are no longer accepting changes that are not critical bug fixes into this project.
See Long Term Support for more detail.

PR Checklist

Please check your PR fulfills the following requirements:

  • Does this PR fix a regression since 1.2.0, a security flaw, or a problem caused by a new browser version?
  • The commit message follows our guidelines
  • Tests for the changes have been added or this is not a bug fix / enhancement
  • Docs have been added, updated, or were not required

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Enhancement
[ ] Documentation content changes
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[x] CI related changes
[ ] Infrastructure changes
[x] Other... Please describe: Fixing the output for scss builds

What is the current behavior?

When building with newer SASS version we get this deprecation message and many others :

DEPRECATION WARNING: Using / for division is deprecated and will be removed in Dart Sass 2.0.0.

Recommendation: math.div($button-fab-height - $button-fab-padding, 2)

More info and automated migrator: https://sass-lang.com/d/slash-div

    ╷
345 │   @include fab-position(bottom-right, auto, ($button-fab-width - $button-fab-padding)/2, ($button-fab-height - $button-fab-padding)/2, auto);
    │                                                                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    ╵
    node_modules\angular-material\angular-material.scss 345:90  fab-all-positions()
    node_modules\angular-material\angular-material.scss 2000:5  @import

Fixes #

What is the new behavior?

This changes the division notation in scss files so that it fits the new expected writing.

These changes where made using the sass-migrator that sass themselves are telling to use.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@google-cla google-cla bot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Jul 5, 2021
@Splaktar Splaktar self-assigned this Jul 5, 2021
@Splaktar Splaktar added this to the 1.2.3 milestone Jul 5, 2021
@Splaktar Splaktar added ui: CSS P3: important Important issues that really should be fixed when possible. labels Jul 5, 2021
Copy link
Member

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

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

These changes generally look fine, however, this is blocked by updating to gulp-sass v5.0.0 and that is blocked by updating to gulp v4, which we never intended to do...

@Splaktar Splaktar added the Blocked Progress on this issue is blocked. Primarily used for PRs that are blocked by presubmit feedback. label Jul 5, 2021
@superheri
Copy link
Contributor Author

Is there a way we could make it work without updating gulp? Because this will invevitably make it impossible to update sass for projects in the future which is a big problem. Or at least apply a build task at the end which run the command of sass-migrator?

@Splaktar
Copy link
Member

Splaktar commented Jul 5, 2021

I've opened #12113 for updating to gulp@4 and linked to some resources for migrating, but unfortunately the gulp team has yet to publish a comprehensive guide to migrating from gulp@3.

I've also opened #12114 to track the need to update to gulp-sass@5 and link to how gulp@4 is required and blocking.

@Splaktar
Copy link
Member

Splaktar commented Jul 5, 2021

I don't think that we want to include sass-migrator in our build pipeline. Users who depend upon our Sass directly, could add that to their build pipeline.

I would like to get onto a supported version of Sass (sass) rather than being stuck on the deprecated node-sass. I think that it could fit within our LTS.

Additionally, gulp@3 has many security vulnerabilities, so updating to gulp@4 should also be possible to fit into our LTS. However, the effort here is pretty significant. Thus I think that it would be more about whether this could actually be completed versus whether it would be accepted during the LTS.

@Splaktar Splaktar modified the milestones: 1.2.3, 1.2.4 Jul 5, 2021
@Splaktar Splaktar modified the milestones: 1.2.4, - Backlog Oct 28, 2021
@Splaktar Splaktar modified the milestones: - Backlog, 1.2.4 Nov 29, 2021
@Splaktar Splaktar removed the Blocked Progress on this issue is blocked. Primarily used for PRs that are blocked by presubmit feedback. label Nov 29, 2021
@Splaktar
Copy link
Member

Splaktar commented Nov 29, 2021

@superheri This should no longer be blocked as I was able to land a fix for #12114. Can you please rebase this PR? That should get the CI passing.

@Splaktar Splaktar added the needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved label Nov 29, 2021
@google-cla google-cla bot added cla: no PR author needs to sign Google's CLA: https://opensource.google.com/docs/cla/ and removed cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ labels Nov 30, 2021
@superheri
Copy link
Contributor Author

It seems like the rebase is not fixing it... Did I do anything wrong?

Sorry for the CLA, the thing is related with my account emails that have changed recently, I'm looking to fix this.

@google-cla google-cla bot added cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ and removed cla: no PR author needs to sign Google's CLA: https://opensource.google.com/docs/cla/ labels Nov 30, 2021
@Splaktar
Copy link
Member

OK, yeah there is still this issue:

PluginError: dist/layouts/angular-material.layout-attributes.scss
Error: @use rules must be written before any other rules.

@Splaktar
Copy link
Member

I can reproduce locally, I'll try to investigate.

@Splaktar
Copy link
Member

Splaktar commented Nov 30, 2021

When we build dist/layouts/angular-material.layout-attributes.scss in

streams.push(
gulp.src(config.scssLayoutAttributeFiles)
.pipe(concat('angular-material.layout-attributes.scss'))
.pipe(sassUtils.hoistScssVariables())

Due to using concat(), we end up with dist/layouts/angular-material.layout-attributes.scss having @use "sass:math"; on line 145 and line 565. We'll need to add a function to be able to

  1. remove duplicate @use statements
  2. move all @use statements to the top

This is likely something similar to the hoistScssVariables() function used here.

@superheri
Copy link
Contributor Author

superheri commented Nov 30, 2021

From what I see, almost all chunks of CSS produced starts with the _variables.scss file. Maybe adding the @use at the top of this file and setting it as the top most file for the only case that is not might fix our problem?

@superheri
Copy link
Contributor Author

Finally it seems to work but we really need to test correctly to make sure I did not break anything.

@Splaktar
Copy link
Member

While you are in there, can you please remove 'src/core/style/color-palette.scss' from here since it no longer exists:

material/gulp/config.js

Lines 97 to 99 in b391c68

scssBaseFiles: [
'src/core/style/color-palette.scss',
'src/core/style/_variables.scss',

@Splaktar
Copy link
Member

When I run the docs locally with your latest changes, I see the following working fine

  • General layout
  • Layout demos
  • General styling
  • Styling of demos, docs, API docs, etc

So this looks good to me so far. I'll have to review the diffs of the .scss files after this gets merged and the snapshot is pushed to bower-material.

@Splaktar Splaktar removed the needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved label Nov 30, 2021
Copy link
Member

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

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

LGTM

@Splaktar Splaktar added the pr: merge ready This PR is ready for a caretaker to review label Nov 30, 2021
@Splaktar Splaktar changed the title build: Fix SCSS division deprecation build: fix SCSS division deprecation Nov 30, 2021
@Splaktar Splaktar added P2: required Issues that must be fixed. and removed P3: important Important issues that really should be fixed when possible. labels Nov 30, 2021
@Splaktar
Copy link
Member

Thank you for your contribution here! Much appreciated.

Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

LGTM

@superheri
Copy link
Contributor Author

Thanks for taking the time to help me with this change.

If there are anything about this I need to change, just tell me.

@Splaktar Splaktar merged commit b5a1a02 into angular:master Nov 30, 2021
@Splaktar
Copy link
Member

Looks like we broke the module builds, I'll look into that and open a separate PR.

Splaktar added a commit that referenced this pull request Dec 15, 2021
- fixes module builds broken by PR #12112
- break module imports out into a separate file since it behaves differently than
  Sass variables.
- add a new `hoistScssAtUseStatements()` function to deduplicates @use statements
  and move them to the top as required by Sass.
- start testing the module and closure builds for every PR and don't let breaking
  changes get to `master` before we find them.
- update CircleCI NodeJS image and comments about where to find new images
- update caniuse-lite
Splaktar added a commit that referenced this pull request Dec 15, 2021
- fixes module builds broken by PR #12112
- break module imports out into a separate file since it behaves differently than
  Sass variables.
- add a new `hoistScssAtUseStatements()` function to deduplicates @use statements
  and move them to the top as required by Sass.
- start testing the module and closure builds for every PR and don't let breaking
  changes get to `master` before we find them.
- update CircleCI NodeJS image and comments about where to find new images
- update caniuse-lite
@Splaktar Splaktar mentioned this pull request Dec 15, 2021
4 tasks
Splaktar added a commit that referenced this pull request Dec 15, 2021
- fixes module builds broken by PR #12112
- break module imports out into a separate file since it behaves differently than
  Sass variables.
- add a new `hoistScssAtUseStatements()` function to deduplicates @use statements
  and move them to the top as required by Sass.
- start testing the module and closure builds for every PR and don't let breaking
  changes get to `master` before we find them.
- update CircleCI NodeJS image and comments about where to find new images
- update caniuse-lite
@Splaktar
Copy link
Member

PR #12138 fixes the module builds.

Splaktar added a commit that referenced this pull request Dec 16, 2021
- fixes module builds broken by PR #12112
- break module imports out into a separate file since it behaves differently than
  Sass variables.
- add a new `hoistScssAtUseStatements()` function to deduplicates @use statements
  and move them to the top as required by Sass.
- start testing the module and closure builds for every PR and don't let breaking
  changes get to `master` before we find them.
- update CircleCI NodeJS image and comments about where to find new images
- update caniuse-lite
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ P2: required Issues that must be fixed. pr: merge ready This PR is ready for a caretaker to review ui: CSS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants