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

Replace sassc with sass-embedded #2551

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

ntkme
Copy link

@ntkme ntkme commented Jul 7, 2022

The sassc gem (libsass) has been deprecated in Oct 2020. sass-embedded is a new ruby api implementation that uses the official dart-sass compiler internally.

This PR replaces sassc with new sass-embedded, so that users can use up to date dart-sass based implementation with latest features such as @use.


Because this PR is targeting 5.x, I decided to make some breaking change in terms of how config works. Now it is just a top level hash config[:sass], which user can set any config key that sass-embedded supports. For example:

# @see https://rubydoc.info/gems/sass-embedded/Sass#compile_string-class_method
set :sass {
  load_paths: [
    File.join(root, 'assets', 'stylesheets'),
    File.join(root, 'my-vendor', 'stylesheets')
  ],
  source_map: true,
  source_map_include_sources: true
}

config[:sass][:quiet_deps] = true

@tnir
Copy link
Contributor

tnir commented Jul 14, 2022

👍
Once this is merged to 5.x, I hope this is backported to 4.x.

Gemfile Outdated Show resolved Hide resolved
@ntkme ntkme force-pushed the sass-embedded branch 3 times, most recently from 97ea908 to 5ea6567 Compare July 22, 2022 19:50
@stale
Copy link

stale bot commented Nov 3, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Nov 3, 2022
@stale stale bot removed the stale label Nov 3, 2022
@tdreyno tdreyno changed the base branch from master to 5.x February 19, 2023 20:21
@ntkme ntkme marked this pull request as draft December 29, 2023 06:22
@ntkme ntkme force-pushed the sass-embedded branch 6 times, most recently from acc2818 to a67b806 Compare January 2, 2024 22:53
@ntkme ntkme marked this pull request as ready for review January 2, 2024 22:57
@ntkme
Copy link
Author

ntkme commented Jan 2, 2024

@tdreyno This PR is ready for review. Please let me know if you have any feedbacks or questions.

@ntkme
Copy link
Author

ntkme commented Jan 18, 2024

Not sure what's the issue with failed macos ruby 3.1 CI test. The issue does not seem to be related to this change. Also, all tests worked fine for me on linux ruby 3.1 locally.

@tdreyno
Copy link
Member

tdreyno commented Jan 26, 2024

5.x is probably abandoned. Commented on not breaking config api compat. Might be best to re-target at 4.x/main

@ntkme ntkme changed the base branch from 5.x to main January 27, 2024 02:57
@ntkme
Copy link
Author

ntkme commented Jan 27, 2024

Note, this PR relies on sass-embedded ~> 1.70, which requires ruby >= 3.1.

@ntkme ntkme requested a review from tdreyno January 27, 2024 03:11
@markets
Copy link
Member

markets commented Jan 30, 2024

@ntkme Do you think we can use a version which supports current CI matrix (Ruby 2.5+)? The idea is to keep compatibility for all those versions, so if that is not possible, we'll probably need to keep both code branches under some check.

@tdreyno what do you think?

@ntkme
Copy link
Author

ntkme commented Jan 31, 2024

In general, I'm in favor of dropping support of end-of-life ruby versions, as they become burden of adopt new features.

The compatibility policy of sass-embedded gem is that, at minimum it would target all MRI releases in "normal maintenance" status (see https://www.ruby-lang.org/en/downloads/branches/), plus the latest JRuby stable branch. When sass-embedded=1.70.0 was released, all MRI versions < 3.0 were end of life, 3.0 was in security maintenance and would be end of life in 2 months, and JRuby was at 9.4, which is MRI 3.1 compatible. Therefore, that release was targeting MRI 3.1.

Ruby 2.5 was never supported in any stable versions of sass-embedded. The very last version supporting ruby 2.6 was 1.58.3, which is missing lots of features and bug fixes. Specifically, there are also a few features added recently to make this middleman PR smaller.


An alternative is to still install sassc by default, and create a new option that allows user to opt-in with using sass-embedded. For reference, for Jekyll we went with this approach for jekyll-sass-converter 2.2.0, and then later dropped sassc completely in jekyll-sass-converter 3.0.0. jekyll 4.3 is locked to "jekyll-sass-converter", ">= 2.0", "< 4.0", so that user can still choose their preferred implementation by choosing between jekyll-sass-converter 2.x or 3.x in Gemfile.

I'm not recommending this option, because it's a lot of extra work supporting end-of-life software.

@tdreyno
Copy link
Member

tdreyno commented Feb 2, 2024

I would recommend having two renderers. They get enabled if the gem they depend on is present (there's other examples of this in the code where adding a templating engine not in the default gem spec will enable support).

https://github.com/middleman/middleman/blob/main/middleman-core/lib/middleman-core/core_extensions/rendering.rb#L9-L13

If the require throws (missing gem), the renderer will not enable.

The concept of "end of life" doesn't make a lot of sense for pieces of tooling like Middleman. Folks have 15 year old blogs running just fine and they shouldn't need to worry about upgrading ruby. Old code should continue to just work.

We can update the new project template to include sass-embedded for new projects and users who want to and can switch can add it to their gem file. We can also bump the minor version as a way to treat this as a new feature and encourage upgrades

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.

None yet

4 participants