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

Ensure return from CSS function #3146

Merged
merged 1 commit into from
Mar 20, 2019

Conversation

fastjames
Copy link
Contributor

Description
When compiling SCSS, the newly included sassc compiler seems to be more vigilant about logic checks. In particular, it returns an error if a CSS function ends without a @return statement. The very-light function has the possibility of doing that, which causes such an error when we test our solidus app against v2.8.2.

I am not sure if it's possible to write tests for css functions, and I do not see any existing tests that appear to target them so I assume that's not a requirement right now.

Checklist:

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have updated Guides and README accordingly to this change (if needed)
  • I have added tests to cover this change (if needed)

According to the `sassc` compiler, all CSS functions should ensure that
they do not end without a `@return` statement. The `very-light` function
has the possibility that it could exit without calling such a statement,
so we add a catchall `@return` at the end to return the unmodified color
value.
Copy link
Contributor

@jacobherrington jacobherrington left a comment

Choose a reason for hiding this comment

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

Make sense to me. Thanks!

@kennyadsl
Copy link
Member

@fastjames thanks! Can you please post the error you are receiving as well?

Copy link
Contributor

@aitbw aitbw left a comment

Choose a reason for hiding this comment

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

Looks like this change is needed as per libsass v3.5, after this patch was merged.

I'd need to see the error that you and your team found for confirmation, but this looks reasonable to me. Thanks @fastjames 💪

@fastjames
Copy link
Contributor Author

Sorry for the delay, I have been unable to get back to this today to reproduce and record the error. Will do so as soon as I can.

Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

We assume the error is the one reported by @aitbw

@kennyadsl kennyadsl merged commit 2ac5bb8 into solidusio:master Mar 20, 2019
@fastjames fastjames deleted the ensure_css_return branch March 20, 2019 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants