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

Drop support for sassc #140

Merged
merged 2 commits into from Sep 14, 2022
Merged

Drop support for sassc #140

merged 2 commits into from Sep 14, 2022

Conversation

ntkme
Copy link
Contributor

@ntkme ntkme commented Sep 6, 2022

My proposal would be to produce a new MAJOR version of jekyll-sass-converter that:

  1. Defaults to the sass-embedded implementation
  2. Changes this import behavior
  3. Removes the sassc compatibility entirely
  4. Documents the upgrade path

Per above comment and discussion in #137, this PR drops support for sassc.

Changes

  • Remove sassc.
  • Bump required_ruby_version to >= 2.6.0 as it is a requirement for sass-embedded.
  • Drop the undocumented add_charset option. Whether to add @charset or not will now be automatically determined based on associate_page_failed?. If a page is associated, sass will automatically add @charset or BOM if output contains any non-ASCII character. Otherwise, e.g. when called via sassify/scssify filters, @charset or BOM will be omitted.
  • Generate embedded base64 source map for sassify/scssify filters if source map is enabled.
  • Cherry-pick source map relative path change from Fix sassc relative import from input file #137.

@ashmaroli
Copy link
Member

Thank you for getting the ball rolling towards a major version bump, @ntkme. I would like to see the Readme explicitly document the different out-of-the-box behaviors from v2.x and v3.x by the time this is ready for merge.
Once again, thanks for the huge effort put in already. 👍

@ntkme
Copy link
Contributor Author

ntkme commented Sep 7, 2022

@ashmaroli @parkr Added a migration section in README and ready for initial review.

@ashmaroli
Copy link
Member

@ntkme Is there a test for inline sourcemap with Base64 data..? I can't seem to find it.. if there is no test, it needs to be added as it is a new feature.

@ntkme
Copy link
Contributor Author

ntkme commented Sep 12, 2022

@ashmaroli Finally got sometime to add tests around inline sourcemap.

@ashmaroli
Copy link
Member

@ntkme I apologise for not telling you this earlier but I think you should extract the inline-sourcemap related changes to another branch and have it merged via a separate pull request that is meant to be merged at a future date.
Let this current pull request be just about removing legacy stuff and associated refactors.

@ntkme
Copy link
Contributor Author

ntkme commented Sep 13, 2022

@ashmaroli Took it out.

@ashmaroli
Copy link
Member

ashmaroli commented Sep 13, 2022

Is the following code meant to be in the current PR?

# Returns a source mapping url for given source-map.
def source_mapping_url(source_map)
  if associate_page_failed?
    "data:application/json;base64,#{Base64.strict_encode64(source_map)}"
  else
    Addressable::URI.encode(sass_page.basename + ".css.map")
  end
end

This looks like another enhancement to me..

@ntkme
Copy link
Contributor Author

ntkme commented Sep 13, 2022

@ashmaroli Done.

Copy link
Member

@ashmaroli ashmaroli left a comment

Choose a reason for hiding this comment

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

LGTM!

@ashmaroli
Copy link
Member

Thanks @ntkme
@jekyllbot: merge +major

@jekyllbot jekyllbot merged commit 5735f01 into jekyll:master Sep 14, 2022
jekyllbot added a commit that referenced this pull request Sep 14, 2022
@ntkme ntkme deleted the drop-sassc branch September 14, 2022 15:12
@jekyll jekyll locked and limited conversation to collaborators Sep 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants