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: Switch to dart-sass #496

Closed
wants to merge 4 commits into from
Closed

Conversation

sebpearce
Copy link
Contributor

@sebpearce sebpearce commented May 19, 2020

This PR was split off from #317, with the intention of shipping the dart-sass change before attempting any changes to our Sass code.

Dart-sass gives us various benefits like the ability to use @use, a sensible alternative to @import that supports namespacing, forcing explicit imports, etc.

image

Changes

  • Replace node-sass with sass (an implementation of dart-sass) in the:
    • root of Kaizen,
    • Kaizen Site,
    • Storybook config.

Info

  • The API of dart-sass is said to be the same as node-sass, so I don't expect much friction.

  • We should however expect slightly slower compile times after the switch (see below).

From the sass package page on npm:

image

@ActuallyACat
Copy link
Contributor

Thinking out loud here.

Since we don't compile our CSS as soon as we use use or any other feature of Sass that Dart Sass supports and Node Sass doesn't we're going to get issues in Murmur/PUI.

Perhaps this is the right time to consider compiling our Sass into CSS? We won't be restricted by the Sass implementations of other repos in this scenario.

@sebpearce
Copy link
Contributor Author

sebpearce commented May 19, 2020

Since we don't compile our CSS as soon as we use use or any other feature of Sass that Dart Sass supports and Node Sass doesn't we're going to get issues in Murmur/PUI.

Correct – the current plan is to switch all repos over to dart-sass before introducing anything like @use. I've already got a PR for Murmur and am about to do the others.

Switching from compiling Sass to CSS may be a good idea but it seems like a much bigger project than I've budgeted myself this week :( E.g. There's a large number of cases where Murmur is importing our Sass and using its vars, so it'd be a lot of work to clean all that up. Would need a focused group effort I think (possible pitch?).

@ActuallyACat
Copy link
Contributor

Correct – the current plan is to switch all repos over to dart-sass before introducing anything like @use.

The risk is something might slip through, and we won't realise until the other camps complain. If that's a risk we're willing to take it it would be good to see a plan for either:

  1. Create space to compile Sass into CSS (next cycle cooldown or a pitch), or
  2. Get Dart Sass on the radar of engineers in PUI/Murmur

@sebpearce
Copy link
Contributor Author

The risk is something might slip through

I'm not sure what you mean here. Do you mean someone might accidentally use @use in Kaizen in the period of time between this PR merging and the other repos switching to dart-sass?

@ActuallyACat
Copy link
Contributor

ActuallyACat commented May 19, 2020

Do you mean someone might accidentally use @use in Kaizen in the period of time between this PR merging and the other repos switching to dart-sass?

Yup, exactly. If we can't use the features of Dart Sass then this is just adding risk with no benefits (unless I've missed something)

@sebpearce
Copy link
Contributor Author

sebpearce commented May 19, 2020

I see. I was thinking I was mitigating risk by doing all the repos over the course of 2 days or so, and only then considering using @use in Kaizen. I don't see how we can get to a point where we can use dart-sass without this step.

Would it help if we edited the CODEOWNERS so that any change to Kaizen (including the old drafts) go through us? I can personally inspect every PR to look for @use and make sure nobody's introduced it. By the end of the week I'm expecting to have the other repos on dart-sass so it'd be temporary.

@sebpearce
Copy link
Contributor Author

Update: We followed up on the above conversation, and agreed that the right approach would be:

  • Get dart-sass running on all consuming repos first
  • Involve engineers in those repos so that they're across any changes
  • Investigate any potential compile time hit and discuss it with affected engineers before merging anything
  • After all consumers are on dart-sass, switch Kaizen to it and only then can we introduce @use.

@mbylstra
Copy link
Contributor

mbylstra commented Feb 22, 2021

Update: We followed up on the above conversation, and agreed that the right approach would be:

Get dart-sass running on all consuming repos first
Involve engineers in those repos so that they're across any changes
Investigate any potential compile time hit and discuss it with affected engineers before merging anything
After all consumers are on dart-sass, switch Kaizen to it and only then can we introduce @use.

Is this still the plan? I have a vague memory that there was a reason we completely dropped this project. Was it because we found that dart scss dramatically slows down compilation? (I could be misremembering this).

I guess the fact that consuming repos are still relying on mixins and scss vars more than half the time is a major blocker for compiling to css only (or we remove all the mixins/vars in a huge breaking change release with the likely result of no one ever upgrades to the latest version of Kaizen)

@sebpearce
Copy link
Contributor Author

sebpearce commented Feb 22, 2021

Ah damn, I had meant to follow up here with the results.

Yeah, the compilation time was the issue – sometimes quoted as around 50x. In my case webpack actually ran out of memory and crashed!

There might be some hope though. When I looked through my notes on it just now, I had saved a link to this discussion, which I meant to refer to to show how there was no way for us to use the fast version of Dart Sass, which is the Dart VM, since it wasn't available in JS. But it looks like such a thing may have been created in August, just after I dropped this project – see final comment here...

@mbylstra
Copy link
Contributor

oooh, sounds promising!

@mbylstra
Copy link
Contributor

So using the Dart VM (and whatever infra is required) was off the cards?

@sebpearce
Copy link
Contributor Author

sebpearce commented Feb 22, 2021

So using the Dart VM (and whatever infra is required) was off the cards?

Correct, there was no appetite for adding Dart to our tech stack. And I'm not sure there was even a way to get it to work even if there was appetite, hence the need for something like the wrapper mentioned at the end of that thread.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 1, 2021

This PR hasn't seen activity in two months! Should it be merged, closed, or worked on further? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in a fortnight.

@github-actions github-actions bot added the stale label Sep 1, 2021
@github-actions
Copy link
Contributor

This PR was closed due to 2 months of inactivity. Feel free to reopen it if still relevant.

@github-actions github-actions bot closed this Sep 16, 2021
@HeartSquared HeartSquared deleted the seb/switch-to-dart-sass branch June 21, 2022 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants