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

[WIP] dart-sass compatibility tweaks #4298

Closed
wants to merge 3 commits into from
Closed

[WIP] dart-sass compatibility tweaks #4298

wants to merge 3 commits into from

Conversation

atesgoral
Copy link
Contributor

@atesgoral atesgoral commented Jul 12, 2021

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 for outputStyle 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:
import React from 'react';
import {Page} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      {/* Add the code you want to test in here */}
    </Page>
  );
}

🎩 checklist

@ghost
Copy link

ghost commented Jul 12, 2021

👋 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines.

@atesgoral atesgoral changed the title dart-sass compatibility tweaks [WIP} dart-sass compatibility tweaks Jul 12, 2021
@atesgoral atesgoral changed the title [WIP} dart-sass compatibility tweaks [WIP] dart-sass compatibility tweaks Jul 12, 2021
Copy link
Member

@BPScott BPScott left a 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";
Copy link
Member

@BPScott BPScott Jul 12, 2021

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},
Copy link
Member

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

Copy link
Contributor Author

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";
Copy link
Member

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);
Copy link
Member

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

@atesgoral
Copy link
Contributor Author

@nickpresta Feel free to take this branch over as I had abandoned this effort a while ago...

@BPScott
Copy link
Member

BPScott commented Jan 24, 2022

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.

@BPScott BPScott closed this Jan 29, 2022
@alex-page alex-page deleted the dart-sass branch March 22, 2022 04:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants