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.

Sign up for GitHub

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

Jump to bottom

Migrate to sassc gem #75

Merged
merged 5 commits into from
Dec 11, 2018
Merged

Migrate to sassc gem #75

merged 5 commits into from
Dec 11, 2018

Conversation

pathawks
Copy link
Member

Fixes #74

@pathawks pathawks force-pushed the sassc branch 4 times, most recently from 46a91b1 to ce1fb3d Compare July 21, 2018 00:40
pathawks
pathawks commented Jul 21, 2018
View reviewed changes
lib/jekyll/converters/scss.rb
rescue ::Sass::SyntaxError => e
raise SyntaxError, "#{e} on line #{e.sass_line}"
rescue ::SassC::SyntaxError => e
line = e.instance_variable_get(:@line)
Copy link
Member Author

Choose a reason for hiding this comment

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

Note the use of instance_variable_get to maintain previous functionality 😕

pathawks
pathawks commented Jul 21, 2018
View reviewed changes
spec/sass_converter_spec.rb
@@ -50,7 +50,7 @@ def converter(overrides = {})
end

it "includes the syntax error line in the syntax error message" do
error_message = "Invalid CSS after \"$font-stack\": expected expression (e.g. 1px, bold), was \";\" on line 1"
error_message = %r!\AError: Invalid CSS after "f": expected 1 selector or at-rule. was "font-family: \$font-"\s+on line 1 of stdin!
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that error messages have changed

pathawks
pathawks commented Jul 21, 2018
View reviewed changes
spec/scss_converter_spec.rb
@@ -119,7 +119,7 @@ def converter(overrides = {})
end

it "includes the syntax error line in the syntax error message" do
error_message = 'Invalid CSS after "body ": expected selector or at-rule, was "{" on line 2'
error_message = %r!\AError: Invalid CSS after "body": expected 1 selector or at-rule, was "{"\s+on line 2!
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that error messages have changed

@parkr
Copy link
Member

parkr commented Jul 21, 2018

@jekyll/windows @ashmaroli Can any of you confirm that sassc works ok on windows?

pathawks
pathawks commented Jul 21, 2018
View reviewed changes
lib/jekyll-sass-converter/version.rb Outdated
@@ -1,5 +1,5 @@
# frozen_string_literal: true

module JekyllSassConverter
VERSION = "1.5.2"
VERSION = "1.6.0"
Copy link
Member Author

Choose a reason for hiding this comment

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

This will certainly require a major version bump, but Jekyll 3.x will not load jekyll-sass 2.x

@pathawks
Copy link
Member Author

This breaks uswds-jekyll, which I assume means it breaks any theme with Sass?

Conversion error: Jekyll::Converters::Scss encountered an error while converting 'assets/css/main.scss':
                    Error: File to import not found or unreadable: uswds/all. on line 78 of stdin >> @import 'uswds/all'; ^ on line 78

@pathawks
Copy link
Member Author

ref build time in seconds
sassc 315.84
v1.5.2 323.27

Very minor (~3%) improvement in build time (for the sites that actually build)

@pathawks pathawks added the major label Jul 21, 2018
@ashmaroli
Copy link
Member

Can any of you confirm that sassc works ok on windows?

@parkr I pulled this branch down to check:

  • bundle install: Success! All dependencies installed ✔️
  • bundle exec rspec:
36 examples, 1 failure
rspec ./spec/scss_converter_spec.rb:189 # Jekyll::Converters::Scss importing from external libraries
unsafe mode brings in the grid partial

@ashmaroli

This comment has been minimized.

Sign in to view
@pathawks
Copy link
Member Author

@ashmaroli I don't get that failure on macOS 🤷‍♂️

It would seem to be related to the theme issue though.

@ashmaroli
Copy link
Member

It would seem to be related to the theme issue though.

Running bundle exec rspec on the master returns no failures

@XhmikosR

This comment has been minimized.

Sign in to view
@pathawks

This comment has been minimized.

Sign in to view
@XhmikosR

This comment has been minimized.

Sign in to view
@parkr
Copy link
Member

parkr commented Jul 22, 2018

I added the project in AppVeyor!

@ashmaroli

This comment has been minimized.

Sign in to view
@pathawks

This comment has been minimized.

Sign in to view
@DirtyF DirtyF force-pushed the master branch 2 times, most recently from 5bcd126 to 6fe4f84 Compare July 23, 2018 17:39
@ashmaroli

This comment has been minimized.

Sign in to view
Closed
@DirtyF
chore(dep): bump to sass-c 2.0
3fa4b41 
Remove dependency on Ruby Sass
sass/sassc-ruby#85
DirtyF
DirtyF approved these changes Oct 1, 2018
View reviewed changes
Copy link
Member

@DirtyF DirtyF left a comment •
edited
Loading

Choose a reason for hiding this comment

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

💚

@ashmaroli
Copy link
Member

Looks like sassc-2.0.0 requires a minimum Ruby version of 2.3.3
The gemspec here relaxes it upto 2.3.0.. that needs to be fixed as well..

@DirtyF
Copy link
Member

DirtyF commented Oct 1, 2018

@ashmaroli correct, appveyor is still chocking on the import:

importing from external libraries
    unsafe mode
  Conversion error: Jekyll::Converters::Scss encountered an error while converting 'css/main.scss':
                    Error: File to import not found or unreadable: color. on line 1 of stdin >> @import "color"; ^ on line 1
      brings in the grid partial (FAILED - 1)

@ashmaroli
Copy link
Member

appveyor is still chocking

probably not related to the RUBY_VERSION requirements.
I'll see if I can send a PR for that issue before this gets merged..

@DirtyF
chore(dep): require Ruby 2.3.3 or higher
c4d4e3e 
to match sassc minimum requirements
@ashmaroli

This comment has been minimized.

Sign in to view
@DirtyF DirtyF requested a review from a team November 13, 2018 20:23
@tcyrus
Copy link

tcyrus commented Dec 11, 2018

Is the windows issue related to sass/sassc-ruby#93?

@ashmaroli
Copy link
Member

@tcyrus Yes. It is..

@DirtyF
Copy link
Member

DirtyF commented Dec 11, 2018

@jekyllbot: merge +major

@jekyllbot jekyllbot merged commit 8999817 into master Dec 11, 2018
@jekyllbot jekyllbot deleted the sassc branch December 11, 2018 20:28
jekyllbot added a commit that referenced this pull request Dec 11, 2018
@jekyllbot
Update history to reflect merge of #75 [ci skip]
a7bf60d
@ashmaroli
Copy link
Member

@DirtyF Are you planning to cut a release anytime soon..? 'coz as it stands, this is not compatible with theme-gems on Windows..

@DirtyF
Copy link
Member

DirtyF commented Dec 12, 2018

@ashmaroli No, we have 'til March.

@pikislabis pikislabis mentioned this pull request Jan 11, 2019
Open
@ashmaroli ashmaroli mentioned this pull request Jan 12, 2019
Closed
@eupiteco eupiteco mentioned this pull request Feb 4, 2019
Closed
@DirtyF DirtyF added this to the v2.0 milestone Mar 24, 2019
@jekyll jekyll locked and limited conversation to collaborators Mar 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@DirtyF DirtyF DirtyF approved these changes

@parkr parkr parkr approved these changes

Assignees
No one assigned
Labels
feature frozen-due-to-age major
Projects
None yet
Milestone
v2.0
Development

Successfully merging this pull request may close these issues.

Ruby Sass deprecated in March 2019
7 participants
@pathawks @parkr @ashmaroli @XhmikosR @DirtyF @tcyrus @jekyllbot