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

Support sass-embedded as alternative implementation #124

Merged
merged 33 commits into from
Oct 15, 2021
Merged
Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
b7a6695
Support sass-embedded as alternative implementation
ntkme May 29, 2021
7fd9cb6
Update sass-embedded
ntkme Sep 23, 2021
13c7419
Mark methods as private
ntkme Sep 23, 2021
8ca9d6d
Remove extra blank line
ntkme Sep 24, 2021
3007b82
Have different defaults for two implementations
ntkme Sep 24, 2021
e3cc37c
Assert error messages conditionally
ntkme Sep 24, 2021
e8714e0
Use plain if instead of unless .nil?
ntkme Sep 24, 2021
1caf7e6
Make sure test does not break if SASS_IMPLEMENTATION is not defined
ntkme Sep 24, 2021
1636e1b
Revert docs/_config.yml
ntkme Sep 24, 2021
472c1b3
Rename :file to :file_path
ntkme Sep 24, 2021
e7ca22a
Fix accidentally modified tests
ntkme Sep 24, 2021
174c81d
Print sass-embedded version information
ntkme Sep 24, 2021
de9abbe
Add comment about behavior difference
ntkme Sep 24, 2021
2ddbbfc
Use string implementation name
ntkme Sep 24, 2021
01add9c
Rename variables
ntkme Sep 24, 2021
2eda70b
Add :sass_embedded? test helper
ntkme Sep 24, 2021
fa334af
Document sass-embedded dependency installation
ntkme Sep 25, 2021
942dcfe
Use Jekyll::External.require_with_graceful_fail
ntkme Sep 26, 2021
1e3031f
Require "sass-embedded"
ntkme Sep 26, 2021
f6cf7a1
Update sass-embedded
ntkme Sep 26, 2021
c7188aa
Use log_level error for formatted error message
ntkme Sep 26, 2021
0c0b77d
Update sass-embedded
ntkme Sep 28, 2021
6e14bee
Update sass-embedded
ntkme Sep 29, 2021
62e62b9
Update README.md
ntkme Sep 29, 2021
f991914
Update README.md
ntkme Sep 29, 2021
e012438
Update README.md
ntkme Sep 29, 2021
99faa86
Update Gemfile
ntkme Sep 29, 2021
463adf7
Update README.md
ntkme Sep 29, 2021
2046ba9
Update README.md
ntkme Sep 29, 2021
b9ddc2f
Update README.md
ntkme Sep 29, 2021
55a485f
Update README.md
ashmaroli Sep 29, 2021
8f00f40
Improve documentation on using `sass-embedded`
ashmaroli Sep 29, 2021
bb410b8
Add a comment for default sass style
ntkme Oct 15, 2021
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,5 @@ gemspec

gem "jekyll", ENV["JEKYLL_VERSION"] ? "~> #{ENV["JEKYLL_VERSION"]}" : ">= 4.0"
gem "minima"

gem "sass-embedded", "~> 0.9.1" if RUBY_VERSION >= "2.6.0"
36 changes: 35 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,32 @@ Jekyll Sass Converter comes bundled with Jekyll 2.0.0 and greater. For more
information about usage, visit the [Jekyll Assets Documentation
page](https://jekyllrb.com/docs/assets/).

### Sass Implementations

#### SassC

By default, Jekyll Sass Converter uses [sassc](https://rubygems.org/gems/sassc)
for Sass implmentation. `sassc` is based on LibSass, and
[LibSass is deprecated](https://sass-lang.com/blog/libsass-is-deprecated).

#### Sass Embedded

[sass-embedded](https://rubygems.org/gems/sass-embedded) is a host for the
[Sass embedded protocol](https://github.com/sass/embedded-protocol). The host
runs [Dart Sass compiler](https://github.com/sass/dart-sass-embedded) as a subprocess
and communicates with the dart-sass compiler via message-passing.
ashmaroli marked this conversation as resolved.
Show resolved Hide resolved

`sass-embedded` is currently experimental and unstable. It requires Ruby 2.6 or higher.

To use the `sass-embedded` implementation, you need to add a dependency on the
`sass-embedded` gem. For example, if you're using a Gemfile, run `bundle add sass-embedded`. Then, you'll be able to
specify `sass-embedded` in your `_config.yml`:

```yml
sass:
implementation: sass-embedded
```

### Source Maps

Starting with `v2.0`, the Converter will by default generate a _source map_ file along with
Expand All @@ -53,14 +79,22 @@ Configuration options are specified in the `_config.yml` file in the following w

Available options are:

* **`implementation`**

Sets the Sass implementation to use.
Can be `sassc` or `sass-embedded`.

Defaults to `sassc`.

* **`style`**

Sets the style of the CSS-output.
Can be `nested`, `compact`, `compressed`, or `expanded`.
See the [SASS_REFERENCE](https://sass-lang.com/documentation/cli/dart-sass#style)
for details.

Defaults to `compact`.
Defaults to `compact` for `sassc`.
Defaults to `expanded` for `sass-embedded`.
Copy link
Member

Choose a reason for hiding this comment

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

Why would we have two different defaults for the implementation? That seems counter to what I'd expect.

Copy link
Contributor Author

@ntkme ntkme Oct 14, 2021

Choose a reason for hiding this comment

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

See https://sass-lang.com/documentation/js-api/interfaces/LegacySharedOptions#outputStyle

The output style of the compiled CSS. There are four possible output styles:

  • "expanded" (the default for Dart Sass) writes each selector and declaration on its own line.
  • "compressed" removes as many extra characters as possible, and writes the entire stylesheet on a single line.
  • "nested" (the default for Node Sass, not supported by Dart Sass) indents CSS rules to match the nesting of the Sass source.
  • "compact" (not supported by Dart Sass) puts each CSS rule on its own single line.

The reason is that dart-sass has dropped support for compact and nested, and we don't want to change the default for sassc.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha. What happens if I specify an unsupported output style? Do I see an error? What does that error look like?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/ntkme/sass-embedded-host-ruby/blob/8913a3f521db5c2485344701745bd2839a68a59f/lib/sass/embedded/compile_context.rb#L218

But in this PR we check for the input and fallback to the respective default value so user won’t see any error.


* **`sass_dir`**

Expand Down
69 changes: 64 additions & 5 deletions lib/jekyll/converters/scss.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ class Scss < Converter
end
end

ALLOWED_IMPLEMENTATIONS = %w(sassc sass-embedded).freeze
ntkme marked this conversation as resolved.
Show resolved Hide resolved
ALLOWED_STYLES = %w(nested expanded compact compressed).freeze

# Associate this Converter with the "page" object that manages input and output files for
Expand Down Expand Up @@ -113,9 +114,15 @@ def sass_dir
jekyll_sass_configuration["sass_dir"]
end

def sass_implementation
implementation = jekyll_sass_configuration["implementation"]
ALLOWED_IMPLEMENTATIONS.include?(implementation) ? implementation : "sassc"
Copy link
Member

Choose a reason for hiding this comment

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

Should we send a warn if the implementation is not allowed so the user can fix their config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is aligned with the style option that it fallbacks to default value without warning. Shall we add warning for both?

Copy link
Member

Choose a reason for hiding this comment

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

It makes sense to warn the user that we don't support the value they asked for. Maybe in another PR if you like.

end

def sass_style
style = jekyll_sass_configuration.fetch("style", :compact)
ALLOWED_STYLES.include?(style.to_s) ? style.to_sym : :compact
ntkme marked this conversation as resolved.
Show resolved Hide resolved
default = sass_implementation == "sassc" ? :compact : :expanded
ntkme marked this conversation as resolved.
Show resolved Hide resolved
style = jekyll_sass_configuration.fetch("style", default)
ALLOWED_STYLES.include?(style.to_s) ? style.to_sym : default
end

def user_sass_load_paths
Expand Down Expand Up @@ -179,18 +186,53 @@ def sass_configs
)
end

def sass_embedded_config(data)
{
ashmaroli marked this conversation as resolved.
Show resolved Hide resolved
:data => data,
:file => file_path,
:indented_syntax => syntax == :sass,
:include_paths => sass_load_paths,
:output_style => sass_style,
:source_map => sourcemap_required?,
:out_file => output_path,
:omit_source_map_url => !sourcemap_required?,
:source_map_contents => true,
}
end

def convert(content)
case sass_implementation
when "sass-embedded"
Jekyll::External.require_with_graceful_fail("sass-embedded")
sass_embedded_convert(content)
when "sassc"
sass_convert(content)
end
end

private

def sass_convert(content)
ntkme marked this conversation as resolved.
Show resolved Hide resolved
config = sass_configs
engine = SassC::Engine.new(content.dup, config)
output = engine.render
generate_source_map(engine) if sourcemap_required?
sass_generate_source_map(engine) if sourcemap_required?
replacement = add_charset? ? '@charset "UTF-8";' : ""
output.sub(BYTE_ORDER_MARK, replacement)
rescue SassC::SyntaxError => e
raise SyntaxError, e.to_s
end

private
def sass_embedded_convert(content)
output = ::Sass.render(**sass_embedded_config(content))
sass_embedded_generate_source_map(output.map) if sourcemap_required?
replacement = add_charset? ? '@charset "UTF-8";' : ""
eof = sourcemap_required? ? "" : "\n"
output.css.sub(BYTE_ORDER_MARK, replacement) + eof
rescue ::Sass::Embedded::RenderError => e
Jekyll.logger.error e.formatted
raise SyntaxError, e.to_s
end

# The Page instance for which this object acts as a converter.
attr_reader :sass_page
Expand All @@ -209,6 +251,16 @@ def filename
File.join(site_source_relative_from_pwd, sass_page.name)
end

# The path of the input scss (or sass) file. This information will be used for error
# reporting and will written into the source map file as main source.
#
# Returns the path of the input file or nil if #associate_page failed
def file_path
return nil if associate_page_failed?

File.join(site_source_relative_from_pwd, sass_page.path)
Copy link
Member

Choose a reason for hiding this comment

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

This might need to use Jekyll.sanitized_path, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be safe as:

  • File.join is simply: base + '/' + path. It is concatenated as is without any expansion.
  • sass_page.path is the path to sass page e.g. css/main.scss - this is provided by jekyll. Since jekyll only process files inside current project, this should be safe to use.

Copy link
Member

Choose a reason for hiding this comment

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

What happens for symlinks?

Copy link
Contributor Author

@ntkme ntkme Oct 15, 2021

Choose a reason for hiding this comment

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

@parkr I just tested and Jekyll.sanitized_path does not do anything different for symlinks. If file is a symlink, sass_page.path still returns the original unresolved path.

In the end when Jekyll process "pages" collection, it does not seem to care if a "page" is a symlink to a file outside of the project or not. I would say that is a problem of Jekyll pages collection, not something we should deal with here.

end

# The value of the `line_comments` option.
# When set to `true` causes the line number and filename of the source be emitted into the
# compiled CSS-file. Useful for debugging when the source-map is not available.
Expand Down Expand Up @@ -266,7 +318,7 @@ def source_map_page
# Reads the source-map from the engine and adds it to the source-map-page.
#
# @param [::SassC::Engine] engine The sass Compiler engine.
def generate_source_map(engine)
def sass_generate_source_map(engine)
return if associate_page_failed?

source_map_page.source_map(engine.source_map)
Expand All @@ -275,6 +327,13 @@ def generate_source_map(engine)
Jekyll.logger.warn "Could not generate source map #{e.message} => #{e.cause}"
end

def sass_embedded_generate_source_map(source_map)
return if associate_page_failed?

source_map_page.source_map(source_map)
site.pages << source_map_page
end

def site
if associate_page_failed?
Jekyll.sites.last
Expand Down
11 changes: 10 additions & 1 deletion script/test
Original file line number Diff line number Diff line change
@@ -1,2 +1,11 @@
#!/bin/bash
bundle exec rspec $@

set -e

echo "Running rspec with sassc"
SASS_IMPLEMENTATION=sassc bundle exec rspec $@

if bundle info sass-embedded; then
echo "Running rspec with sass-embedded"
SASS_IMPLEMENTATION=sass-embedded bundle exec rspec $@
fi
33 changes: 24 additions & 9 deletions spec/sass_converter_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,16 @@
SASS
end

let(:css_output) do
let(:expanded_css_output) do
<<~CSS
body {
font-family: Helvetica, sans-serif;
font-color: fuschia;
}
CSS
end

let(:compact_css_output) do
<<~CSS
body { font-family: Helvetica, sans-serif; font-color: fuschia; }
CSS
Expand Down Expand Up @@ -51,15 +60,21 @@ def converter(overrides = {})

context "converting sass" do
it "produces CSS" do
expect(converter.convert(content)).to eql(css_output)
expected = sass_embedded? ? expanded_css_output : compact_css_output
expect(converter.convert(content)).to eql(expected)
end

it "includes the syntax error line in the syntax error message" do
error_message = 'Error: Invalid CSS after "f": expected 1 selector or at-rule.'
error_message = %r!\A#{error_message} was "font-family: \$font-"\s+on line 1:1 of stdin!
expected = if sass_embedded?
%r!Expected newline!i
else
error_message = 'Error: Invalid CSS after "f": expected 1 selector or at-rule.'
%r!\A#{error_message} was "font-family: \$font-"\s+on line 1:1 of stdin!
end

expect do
converter.convert(invalid_content)
end.to raise_error(Jekyll::Converters::Scss::SyntaxError, error_message)
end.to raise_error(Jekyll::Converters::Scss::SyntaxError, expected)
end

it "removes byte order mark from compressed Sass" do
Expand All @@ -81,7 +96,7 @@ def converter(overrides = {})
make_site(
"source" => File.expand_path("pages-collection", __dir__),
"sass" => {
"style" => :compact,
"style" => :expanded,
},
"collections" => {
"pages" => {
Expand All @@ -93,7 +108,7 @@ def converter(overrides = {})

it "produces CSS without raising errors" do
expect { site.process }.not_to raise_error
expect(sass_converter.convert(content)).to eql(css_output)
expect(sass_converter.convert(content)).to eql(expanded_css_output)
end
end

Expand All @@ -102,14 +117,14 @@ def converter(overrides = {})
make_site(
"source" => File.expand_path("[alpha]beta", __dir__),
"sass" => {
"style" => :compact,
"style" => :expanded,
}
)
end

it "produces CSS without raising errors" do
expect { site.process }.not_to raise_error
expect(sass_converter.convert(content)).to eql(css_output)
expect(sass_converter.convert(content)).to eql(expanded_css_output)
end
end
end
52 changes: 37 additions & 15 deletions spec/scss_converter_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,16 @@
SCSS
end

let(:css_output) do
let(:expanded_css_output) do
<<~CSS
body {
font-family: Helvetica, sans-serif;
font-color: fuschia;
}
CSS
end

let(:compact_css_output) do
<<~CSS
body { font-family: Helvetica, sans-serif; font-color: fuschia; }
CSS
Expand Down Expand Up @@ -112,8 +121,9 @@ def converter(overrides = {})
expect(verter.sass_configs[:style]).to eql(:compressed)
end

it "defaults style to :compact" do
expect(verter.sass_configs[:style]).to eql(:compact)
it "defaults style to :expanded for sass-embedded or :compact for sassc" do
expected = sass_embedded? ? :expanded : :compact
expect(verter.sass_configs[:style]).to eql(expected)
end

it "at least contains :syntax and :load_paths keys" do
Expand All @@ -124,14 +134,19 @@ def converter(overrides = {})

context "converting SCSS" do
it "produces CSS" do
expect(converter.convert(content)).to eql(css_output)
expected = sass_embedded? ? expanded_css_output : compact_css_output
expect(converter.convert(content)).to eql(expected)
end

it "includes the syntax error line in the syntax error message" do
error_message = 'Error: Invalid CSS after "body": expected 1 selector or at-rule, was "{"'
error_message = %r!\A#{error_message}\s+on line 2!
expected = if sass_embedded?
%r!expected ";"!i
else
error_message = 'Error: Invalid CSS after "body": expected 1 selector or at-rule'
%r!\A#{error_message}, was "{"\s+on line 2!
end
expect { scss_converter.convert(invalid_content) }.to(
raise_error(Jekyll::Converters::Scss::SyntaxError, error_message)
raise_error(Jekyll::Converters::Scss::SyntaxError, expected)
)
end

Expand Down Expand Up @@ -197,9 +212,13 @@ def converter(overrides = {})

it "brings in the grid partial" do
site.process
expect(File.read(test_css_file)).to eql(
"a { color: #999999; }\n\n/*# sourceMappingURL=main.css.map */"
)

expected = if sass_embedded?
"a {\n color: #999999;\n}\n\n/*# sourceMappingURL=main.css.map */"
else
"a { color: #999999; }\n\n/*# sourceMappingURL=main.css.map */"
end
expect(File.read(test_css_file)).to eql(expected)
end

context "with the sass_dir specified twice" do
Expand Down Expand Up @@ -326,7 +345,7 @@ def converter(overrides = {})
make_site(
"source" => File.expand_path("pages-collection", __dir__),
"sass" => {
"style" => :compact,
"style" => :expanded,
},
"collections" => {
"pages" => {
Expand All @@ -338,7 +357,7 @@ def converter(overrides = {})

it "produces CSS without raising errors" do
expect { site.process }.not_to raise_error
expect(scss_converter.convert(content)).to eql(css_output)
expect(scss_converter.convert(content)).to eql(expanded_css_output)
end
end

Expand All @@ -347,14 +366,14 @@ def converter(overrides = {})
make_site(
"source" => File.expand_path("[alpha]beta", __dir__),
"sass" => {
"style" => :compact,
"style" => :expanded,
}
)
end

it "produces CSS without raising errors" do
expect { site.process }.not_to raise_error
expect(scss_converter.convert(content)).to eql(css_output)
expect(scss_converter.convert(content)).to eql(expanded_css_output)
end
end

Expand Down Expand Up @@ -388,7 +407,10 @@ def converter(overrides = {})

it "contains relevant sass sources" do
sources = sourcemap_data["sources"]
expect(sources).to include("main.scss")
ntkme marked this conversation as resolved.
Show resolved Hide resolved
# sass-embedded (dart-sass) does not inlcude main.scss in sources
# because main.scss only contains @import statements
# thus there is no actual scss code to be mapped
expect(sources).to include("main.scss") unless sass_embedded?
expect(sources).to include("_sass/_grid.scss")
expect(sources).to_not include("_sass/_color.scss") # not imported into "main.scss"
end
Expand Down
Loading