-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[WIP] dart-sass compatibility tweaks #4298
Conversation
👋 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay for preparing to move to dart-sass. However there are compatibility issues with putting dart-sass only code into the public API.
We'll either need to come up with some work arounds to support usage in both node-sass and dart-sass, or remove the problematic parts of the public API before moving to dart-sass
@@ -1,4 +1,6 @@ | |||
// stylelint-disable-next-line scss/partial-no-import | |||
@use "sass:math"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to be an issue.
Code in src/components
all get compiled and shipped as css so we can author those files in whatever format we want, no problems there.
Code in src/styles
(or more strictly, code in files imported from src/styles/_public-api.scss
) however is the Polaris public sass API, and is used by consuming apps. This means that any code in these files needs to be understood by consuming apps. Introducing @use
- dart-sass only syntax - here is a breaking change as it means that consumers will no longer be able to author app scss in node-sass. We create a hard dependency of:this version of polaris ships mixins with dart-sass specific syntax and thus can only be use in apps that compile their scss with dart-sass.
In order to not require a specific major version of polaris to be tied to a specific version of sewing-kit this either needs to be reworked, and either we remove these utilities from polaris (color() is considered not desired but its usage in consuming apps is pervasive which means it's hard to remove) before performing the migration to dart-sass or we come up with some way to support both node-sass and dart-sass.
On the up-side it's been agreed upon that having a sass API is not desired, and it causes more hassle than desired, however work to actually remove it is slow going.
@@ -14,7 +14,7 @@ $font-family-data: ( | |||
Consolas, | |||
Liberation Mono, | |||
Menlo, | |||
monospace !default}, | |||
monospace}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a straight up bug and we can remove this in a separate pr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -1,3 +1,5 @@ | |||
@use "sass:math"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another case where dart-sass only code is introduced to the public sass API
@@ -22,7 +22,7 @@ export function styles({ | |||
|
|||
const filter = createFilter(include, exclude); | |||
|
|||
const renderSass = promisify(nodeSass.render); | |||
const renderSass = promisify(sass.render); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will want to use sass.renderSync
, which is apparently the faster of the two methods. I did a similar thing in here
@nickpresta Feel free to take this branch over as I had abandoned this effort a while ago... |
Gonna close this. It'll be easier to reopen in the future. Lots of these changes have superseded. See #4725 for thoughts on deprecation warnings. |
WHY are these changes introduced?
Sass is not going away any time soon (~6 mo). It might be still worth updating to dart-sass.
WHAT is this pull request doing?
Taking a stab at updating to dart-sass and fixing compatibility issues that pop up.
Ran automigration for division operators (to convert them to multiplication):
npx sass-migrator division src/**/*.scss
The
"compact"
value foroutputStyle
is not supported by dart-sass."expanded"
is the default. So, just removed the option.How to 🎩
🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines
Copy-paste this code in
playground/Playground.tsx
:🎩 checklist
README.md
with documentation changes