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

3.x: jekyll-sass-converter 2.x support #8939

Open
wants to merge 5 commits into
base: 3.9-stable
Choose a base branch
from

Conversation

parkr
Copy link
Member

@parkr parkr commented Jan 17, 2022

This is a 🙋 feature or enhancement.

Summary

This backports jekyll-sass-converter 2.x support into the Jekyll 3.x release line so that GitHub Pages can upgrade to it (if they so choose).

Context

github/pages-gem#813 (comment)
github/pages-gem#749
jekyll/jekyll-sass-converter#122 (comment)

@parkr
Copy link
Member Author

parkr commented Jan 18, 2022

Psych 4.x is giving us grief. ruby/setup-ruby@v1 action with bundler-cache: true needs to have its RubyGems version upgraded.

@ashmaroli
Copy link
Member

@parkr Are you waiting on ruby/setup-ruby to add support for customizing Rubygems versions..? If you're open to using a fork of that action, I could patch that support in. (It will not be merge-worthy (function signature changes) so won't be submitting a PR upstream.)

Also, may I ask why the workflows are being configured to trigger on events on ["master", "*-stable"] instead of just
["*-stable"]..?

@ashmaroli
Copy link
Member

@parkr When you get some time could you extract the superfluous changes in this branch to separate narrowly-scoped PR(s)?
(The release workflow could be pushed directly to the base branch.)

@parkr parkr force-pushed the 3.x-jekyll-sass-converter-2-support branch from 44ea7f6 to 01d4da7 Compare January 21, 2022 05:34
@ashmaroli ashmaroli added ⏪ backport Changes will be merged into an older stable branch enhancement labels Jan 21, 2022
@ashmaroli
Copy link
Member

@parkr I've cleared CI roadblock on this branch.

Noting a few points as FYI (for folks at GHP):

  • This introduces yet another native extension dependency: "sassc".
  • This changes existing behavior slightly. (Refer changes to Cucumber features in this branch).
  • This removes a public instance method: Jekyll::Theme#configure_sass.

Copy link
Member Author

@parkr parkr left a comment

Choose a reason for hiding this comment

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

Quick questions for you @ashmaroli as comments.

sassc

Adding more C is suboptimal and this PR is actually prompted by someone wanting to use the Dart-Sass implementation. Should we upgrade to dart-sass impl then ship this new version? C extensions are notoriously hard for folks, especially on Windows & M1 Macs where things compile differently than x64 *nix.

@@ -154,7 +154,7 @@ Feature: Rendering
When I run jekyll build
Then I should get a zero exit status
And the _site directory should exist
And I should see ".foo-bar {\n color: red; }" in "_site/index.css"
And I should see ".foo-bar { color: red; }\n\n\/\*# sourceMappingURL=index.css.map \*\/" in "_site/index.css"
Copy link
Member Author

Choose a reason for hiding this comment

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

Source map additions - should we check that those files are also output?

Copy link
Member

Choose a reason for hiding this comment

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

Ideally, that is unnecessary.
The sass-converter plugin already covers testing the generation and rendering of sourcemaps.

@@ -38,12 +37,6 @@ def assets_path
@assets_path ||= path_for "assets".freeze
end

def configure_sass
Copy link
Member Author

Choose a reason for hiding this comment

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

I can't remember - why did we remove this again? jekyll-sass-converter now handles themes itself?

Copy link
Member

Choose a reason for hiding this comment

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

why did we remove this again?

If you look at the method's implementation, it calls the Sass module (from erstwhile gem sass) creating an unhealthy dependency on a library managed by the sass-converter plugin.
By removing the method Jekyll Core has completely yielded all control to the plugin. The converter in turn does everything it's meant to do only interfacing with Jekyll Core via expected and documented APIs (e.g. the Converter API endpoint #convert(content); the Generator API endpoint #generate(site), etc.

jekyll-sass-converter now handles themes itself?

Yes. It does via public method Jekyll::Converters::Scss#sass_load_paths

def sass_load_paths
  paths = user_sass_load_paths + [sass_dir_relative_to_site_source]
  paths << site.theme.sass_path if site.theme&.sass_path
  ...
end

@ashmaroli
Copy link
Member

Should we upgrade to dart-sass impl then ship this new version? C extensions are notoriously hard...

@parkr The yet-to-be-released version of the plugin will be an accommodating version.
It will continue to have sassc as a runtime dependency and continue working as of current latest release. The bundled dart-sass implementation will be an opt-in feature that the user will have to make a conscious decision to enable it.

@ntkme
Copy link
Contributor

ntkme commented Mar 3, 2022

Adding more C is suboptimal and this PR is actually prompted by someone wanting to use the Dart-Sass implementation. Should we upgrade to dart-sass impl then ship this new version? C extensions are notoriously hard for folks, especially on Windows & M1 Macs where things compile differently than x64 *nix.

What's unfortunate here is that dart-sass is not fully backwards compatible with sass/sassc. Most of the sites should work just fine, but there are always some edge cases, meaning we cannot jump directly to dart-sass in pages gem unless they are willing to introduce a breaking change.

@RobertAKARobin
Copy link

It would be great to have this released. I'd love to start using Sass's @use.

@ntkme
Copy link
Contributor

ntkme commented May 10, 2023

Based on my nonscientific research, the compatibility between original ruby sass and dart sass for real world jekyll themes is actually pretty good. I looked at the projects which depend on sass-embedded (https://github.com/ntkme/sass-embedded-host-ruby/network/dependents), and then find out lots of them are using jekyll-sass-converter 3.0.0 for development in Gemfile.lock, but they run classic github pages with jekyll-sass-converter 1.5.2 in production. It turns of most of them build without any issue with either version.

@parkr @ashmaroli I understand that users can use GitHub Action to run latest jekyll and that's the preferred way moving forward. However, given that the real world compatibility is actually pretty good, I want to check if there is still any interest on backporting support for jekyll-sass-converter 2.x/3.x to jekyll 3.x branch.

@ashmaroli
Copy link
Member

Hello @ntkme,
My solitary take on the matter is that I don't intend on backporting (such a major enhancement) to the GitHub Pages ecosystem in light of the following:

  • People using GitHub Pages are a mostly non-tech-oriented. This majority prefers things to just work with minimum intervention.
  • We both know that sass-embedded is not a drop-in replacement for older Sass implementations and involved numerous code-changes to achieve near-compatibility.
  • Imagine being an absolute novice following a GitHub walkthrough to the tee yet ending up with multiple Deprecation warnings from Dart Sass, which leads to panic and additional contingency management.
  • GitHub Pages currently uses the legacy Ruby based Sass.
  • GitHub Pages is a locked environment. Even if we were to release a new version with backported support for sass-embedded, there is the need for github-pages gem to actually pick the new version. Being locked to particular version means users do not have the option to downgrade to previous version to avoid using sass-embedded (for whatever may be the reason).

The best route here is to maintain status quo as possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ⏪ backport Changes will be merged into an older stable branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants