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] Replace node-sass with sass-embedded #7321

Closed
wants to merge 14 commits into from

Conversation

aaronccasanova
Copy link
Member

@aaronccasanova aaronccasanova commented Sep 30, 2022

WHAT is this pull request doing?

This PR replaces node-sass with sass-embedded to align with the recent web upgrade. This was accomplished by updating plugin-styles to use the sass-embedded compileAsync API, running the sass-migrator, and addressing a few minor build errors.

While in draft, I plan to leave the migrated files uncommitted so reviewers have the opportunity to easily see the minimum required updates and run the sass-migrator themselves.

Tophatting

  1. Run the sass-migrator
npx sass-migrator division './src/**/*.scss' --migrate-deps && \                                                                                                                                          
npx sass-migrator media-logic './src/**/*.scss' --migrate-deps && \
npx sass-migrator module './src/**/*.scss' --migrate-deps && \
npx sass-migrator namespace './src/**/*.scss' --migrate-deps && \
npx sass-migrator strict-unary './src/**/*.scss' --migrate-deps
  1. Format the migrated files
yarn prettier './polaris-react/**/*.scss' --write

Note: There is a bug with the sass-migrator incorrectly transforming background shorthand values to math.div(...). After running the above migrations, navigate to the ColorPicker.scss file and change math.div(50%, var(--p-space-4)) back to 50% / var(--p-space-4)

  1. Validate the upgraded files look as expected
  2. Run a build in polaris-react and validate there are no build failures
cd polaris-react && yarn build:js

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

includePaths: [path.dirname(id)],
})
.css.toString();
sassOutput = (await sass.compileAsync(id, {style: 'compressed'})).css;
Copy link
Member

Choose a reason for hiding this comment

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

We currently use the expanded style (i.e. https://unpkg.com/browse/@shopify/polaris@10.4.1/build/esm/styles.css)

I reckon we should stick to that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call! I assumed the equivalent of compact was compressed. Updated: 3467ae2

Copy link
Member

Choose a reason for hiding this comment

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

oh duh, I'm wrong and skim-reading sorry.

In node-sass there were 4 output styles, in dart-sass there's only two. compressed and expanded continue to exist, while compact and and nested were removed (ooold context on what those look like).

I'd lean towards the more human-readable output when we publish to npm so it is easier to identify build output differences over time, so I'd prefer expanded over compressed.

@sam-b-rose sam-b-rose closed this Sep 29, 2023
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