Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(@angular-devkit/build-angular): switch to use Sass modern API #23624

Merged
merged 1 commit into from
Sep 27, 2022

Conversation

alan-agius4
Copy link
Collaborator

@alan-agius4 alan-agius4 commented Jul 22, 2022

Sass modern API provides faster compilations times when used in an async manner.

Application compilation duration Sass API and Compiler
60852ms dart-sass legacy sync API
52666ms dart-sass modern API

Note: https://github.com/johannesjo/super-productivity was used for benchmarking.

Prior art: http://docs/document/d/1CvEceWMpBoEBd8SfvksGMdVHxaZMH93b0EGS3XbR3_Q?resourcekey=0-vFm-xMspT65FZLIyX7xWFQ

BREAKING CHANGE:

  • Deprecated support for tilde import has been removed. Please update the imports by removing the ~.

Before

@import "~font-awesome/scss/font-awesome";

After

@import "font-awesome/scss/font-awesome";
  • By default the CLI will use Sass modern API, While not recommended, users can still opt to use legacy API by setting NG_BUILD_LEGACY_SASS=1.

@alan-agius4 alan-agius4 added the target: minor This PR is targeted for the next minor release label Jul 22, 2022
@alan-agius4 alan-agius4 force-pushed the sass-modern-api branch 5 times, most recently from 4fe1a6c to 2361cc3 Compare July 22, 2022 12:30
@alan-agius4 alan-agius4 marked this pull request as ready for review July 22, 2022 12:51
@alan-agius4 alan-agius4 force-pushed the sass-modern-api branch 5 times, most recently from d01a6d4 to 6c06936 Compare July 26, 2022 09:04
@alan-agius4 alan-agius4 force-pushed the sass-modern-api branch 3 times, most recently from e70491a to 19a69dc Compare July 28, 2022 13:37
@alan-agius4 alan-agius4 added the needs: discussion On the agenda for team meeting to determine next steps label Aug 3, 2022
@alan-agius4 alan-agius4 removed the needs: discussion On the agenda for team meeting to determine next steps label Aug 4, 2022
@alan-agius4 alan-agius4 force-pushed the sass-modern-api branch 7 times, most recently from 740b223 to 80988aa Compare August 5, 2022 13:44
@alan-agius4 alan-agius4 added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Aug 5, 2022
@alan-agius4 alan-agius4 force-pushed the sass-modern-api branch 2 times, most recently from 60f5b60 to 91161d8 Compare August 8, 2022 18:05
@alan-agius4 alan-agius4 removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Aug 9, 2022
@alan-agius4 alan-agius4 requested a review from clydin August 26, 2022 07:35
@alan-agius4 alan-agius4 added action: review The PR is still awaiting reviews from at least one requested reviewer 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 Aug 26, 2022
@alan-agius4 alan-agius4 added this to the v15 Feature Freeze milestone Sep 16, 2022
@alan-agius4 alan-agius4 added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Sep 22, 2022
@alan-agius4 alan-agius4 force-pushed the sass-modern-api branch 3 times, most recently from d0dae7f to 503e917 Compare September 23, 2022 09:34
@alan-agius4 alan-agius4 added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Sep 23, 2022
@alan-agius4 alan-agius4 requested review from clydin and removed request for clydin September 23, 2022 13:48
Copy link
Member

@clydin clydin left a comment

Choose a reason for hiding this comment

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

Should we add a note to the breaking change description about the environment variable?

// scss also from the cwd and project root.
// See: https://github.com/webpack-contrib/sass-loader/blob/997f3eb41d86dd00d5fa49c395a1aeb41573108c/src/utils.js#L307
projectRoot,
path.join(root, 'node_modules'),
Copy link
Member

Choose a reason for hiding this comment

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

This workaround won't support package exports fields nor Yarn PnP. We'll need to do a followup with a fix for this. Probably a custom Sass importer that tries node resolution first from the project root.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I already have some ideas for that.

Sass modern API provides faster compilations times when used in an async manner.

|Application compilation duration | Sass API and Compiler|
|-- | --|
|60852ms | dart-sass legacy sync API|
|52666ms | dart-sass modern API|

Note: https://github.com/johannesjo/super-productivity was used for benchmarking.

Prior art: http://docs/document/d/1CvEceWMpBoEBd8SfvksGMdVHxaZMH93b0EGS3XbR3_Q?resourcekey=0-vFm-xMspT65FZLIyX7xWFQ

BREAKING CHANGE:

- Deprecated support for tilde import has been removed. Please update the imports by removing the `~`.

Before
```scss
@import "~font-awesome/scss/font-awesome";
```

After
```scss
@import "font-awesome/scss/font-awesome";
```

- By default the CLI will use Sass modern API, While not recommended, users can still opt to use legacy API by setting `NG_BUILD_LEGACY_SASS=1`.
@alan-agius4 alan-agius4 added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Sep 27, 2022
@angular-robot angular-robot bot added the feature Issue that requests a new feature label Sep 27, 2022
@alan-agius4 alan-agius4 merged commit 308e3a0 into angular:main Sep 27, 2022
@alan-agius4 alan-agius4 deleted the sass-modern-api branch September 27, 2022 08:05
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker feature Issue that requests a new feature flag: breaking change target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants