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

Migrate to Dart Sass #942

Closed
wants to merge 10 commits into from
Closed

Conversation

BSFishy
Copy link
Contributor

@BSFishy BSFishy commented Jul 31, 2023

Description

Migrate to Dart Sass instead of using node-sass.

A few things:

  • Replace sass-extract with sass-extract-dart to work with Dart Sass
    • Call sass_extract, as Node bindings are a little finicky and async doesn't seem to actually be running in async
  • Fix usage of slash division with math.div, as slash division is deprecated in Dart Sass
  • Use custom exec function in compile script to allow for proper async (Javascript eagerly executes promises, so we can't use execSync or wrap it in a custom promise)
  • Compile Sass and run sass-extract in parallel for easy speedup
  • Add array type to generated JSON typedef (sass-extract-dart supports extracting array types, which sass-extract didn't)

Questions:

  • This fails the linter, as in some places we use #{math.div(...)} syntax. sass-lint isn't able to parse this, where it throws an error. We could solve this in 2 ways: quick and easy or long and maintainable
    • Quick and easy: Split the usage into 2 lines: declare a variable with the value and use the variable. This is quick and easy to implement, but we should plan on using Stylelint at some point.
    • Long and maintainable: migrate to Stylelint. sass-lint in unmaintained and is clearly causing us issues. However, I don't know how much effort it might be to migrate. To compare with the migration that happened in OSD (Sass lint OpenSearch-Dashboards#1413), 240 files changed, but it looks like most of them were fixing linter issues?
  • There are some differences in the generated JSON. Mainly it's supporting arrays and some differences in number handling. This might be breaking, but I think all of these are affected outside of what we should consider public API (https://oui.opensearch.org/1.2/#/guidelines/sass). How should we handle this? Will get a diff soon

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • All tests pass
    • yarn lint
    • yarn test-unit
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Matt Provost <[email protected]>
Signed-off-by: Matt Provost <[email protected]>
scripts/compile-scss.js Outdated Show resolved Hide resolved
scripts/compile-scss.js Outdated Show resolved Hide resolved
scripts/derive-sass-variable-types.js Outdated Show resolved Hide resolved
src/components/form/range/_range_highlight.scss Outdated Show resolved Hide resolved
@joshuarrrr
Copy link
Member

@BSFishy I see you generally went with the math.div(...) syntax. Can we avoid the linting errors for now by using calc( foo / bar) syntax instead? https://sass-lang.com/documentation/breaking-changes/slash-div/

Copy link
Collaborator

@SergeyMyssak SergeyMyssak left a comment

Choose a reason for hiding this comment

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

@BSFishy, really good changes!

  1. The solution provided by @joshuarrrr is working, we can use calc instead of math.div to avoid linter errors
  2. We can indeed migrate to Stylelint, mostly the changes will be regarding automatic fixing by linter
  3. I don't think that this is breaking change, but it would be good to take a look at diff

@gulderov
Copy link
Contributor

gulderov commented Aug 3, 2023

@BSFishy we have proposal #119 for migration to Stylelint. It seems reasonable to separate the migration into two distinct pull requests: one for Dart Sass and the other for Stylelint

@gulderov
Copy link
Contributor

gulderov commented Aug 4, 2023

@BSFishy we have stylelint in pull request #949 to support migration

Signed-off-by: Matt Provost <[email protected]>
Signed-off-by: Matt Provost <[email protected]>
Signed-off-by: Matt Provost <[email protected]>
Signed-off-by: Matt Provost <[email protected]>
Signed-off-by: Matt Provost <[email protected]>
Signed-off-by: Matt Provost <[email protected]>
@BSFishy
Copy link
Contributor Author

BSFishy commented Aug 17, 2023

The more I work on this, the more I realize we will most likely need to profile our sass build to see where we're getting performance hits. I have a feeling it's the way we include files. Either way, I think I'm going to need to put this PR on hold until that gets fixed

@BSFishy BSFishy marked this pull request as draft August 24, 2023 17:15
@BSFishy
Copy link
Contributor Author

BSFishy commented Sep 29, 2023

Superseded by #1001

@BSFishy BSFishy closed this Sep 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants