Skip to content
This repository was archived by the owner on Oct 24, 2025. It is now read-only.

Conversation

@fschwahn
Copy link
Contributor

sassc raises an error when an empty template is passed (see https://github.com/sass/libsass/blob/b2a3d98ed384d2d19aa79157d08fa21c3e8043e0/src/sass_context.cpp#L390). rubysass accepts that however, that means the sass and sassc gems behave differently when passed the same input:

Sass::Engine.new('', style: :compressed).render # => ""
SassC::Engine.new('', style: :compressed).render # raises SassC::SyntaxError
# but with one space:
SassC::Engine.new(' ', style: :compressed).render # => ""

This change just short-circuits the whole Engine#render-method if the template is empty. This is the easiest solution I could see, but maybe I'm missing something.

A side note: While putting this together I noticed that the :quiet-option seems badly named, as it is more like a dry run if I understand it correctly. The quiet option in rubysass actually surpresses warnings, whule the sassc version just does not return the resulting css. This subtle difference might trip someone up. If the current behavior of the quiet option is indeed intentional, then this PR needs to be updated to make the behavior the same for the empty template case.

Choose a reason for hiding this comment

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

Wouldn't it be better to replace it with return '' if @template.empty?? It may seem the same but it isn't. With the current implementation we would get a reference to the original source and maybe change it after calling render causing a side effect on the original source which could cause some unexpected behaviors if one assume render would always return a new string...

Copy link
Member

Choose a reason for hiding this comment

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

How about return @template.dup if @template.empty?

Choose a reason for hiding this comment

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

I just think '' makes it obvious we are returning an empty string...

Copy link
Member

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

maybe that test could be adapted for taking the empty string into account, since the encoding shouldn't matter for empty strings...

bolandrm added a commit that referenced this pull request Jan 11, 2016
Don't raise error when template is empty
@bolandrm bolandrm merged commit ac003ab into sass:master Jan 11, 2016
@fschwahn
Copy link
Contributor Author

Thanks for merging, @rosenfeld has a point about mutating the input. I think both solutions work, however return ''.force_encoding(@template.encoding) if @template.empty? is probably more explicit than return @template.dup if @template.empty?.

It would probably make sense to add tests that the returned empty string is not the same as the input string, as well as that it preserves the encoding.

@bolandrm
Copy link
Member

@fschwahn I merged as some folks were running into this problem. If you want to further improve the code or tests, that's more than welcome - feel free to submit another PR!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants