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

Add support to configure more deprecation warnings from Dart Sass #164

Merged
merged 6 commits into from
Oct 30, 2024

Conversation

ntkme
Copy link
Contributor

@ntkme ntkme commented Oct 27, 2024

This PR adds new options for controlling deprecation messages.

@ashmaroli
Copy link
Member

Thank you for submitting this pull request, @ntkme.
But I have some misgivings that I would like you to address:

  • Let this pull request be only about adding support for the new sass config options. Dropping support for EOL Rubies is not a priority in this plugin (but unavoidable since sass-embedded has already moved to Ruby 3.1+). The other changes you have proposed can be done in a separate pull request. (No compulsion if you don't want to; I will open a PR to commit those maintenance changes).
  • Instead of requiring sass-embedded-1.80+, let the constraint be ~> 1.75 since the project added the deprecations API support in v1.74.1. This gives the users of this plugin some wiggle-space to downgrade to a lower version in case they are not comfortable with sass-embedded-1.80+.
  • Regarding the proposed Ruby code changes, I am not a fan of having a new Array instance allocated unnecessarily for every call to Jekyll::Converters::Scss#convert. Perhaps some refactoring or memoizations?

@ntkme ntkme force-pushed the update-sass-embedded branch from b8ee971 to 6187fb4 Compare October 28, 2024 16:17
@ntkme ntkme changed the title Update sass-embedded with new options and drop EOL ruby versions Bump sass-embedded requirement to ~> 1.75 and add deprecation options Oct 28, 2024
@ntkme
Copy link
Contributor Author

ntkme commented Oct 28, 2024

@ashmaroli I've completed you comment 1 and 2, for the Array it's more of a user friendly thing to make sure the input is an array. If user has a singular string in config, it will be converted to Array automatically.

Caching is a bit tricky, because for each input sass file Jekyll creates a new Converter instance. So, the cache need to be on the site level instead of converter level, because the each of these functions are only called exactly once per sass input file so there is no point to cache locally on the instance.

In reality I doubt how much performance cost this will have because a site usually only have less than 10 sass entrypoint files, so it feels like a premature optimization that does not matter much.

@ashmaroli
Copy link
Member

Thank you @ntkme.
I will get back to these pull requests on Wednesday / Thursday.

Regardless, about the Array() optimisations, I agree that my nitpicks are premature and largely irrelevant. But rather than have RuboCop flag those lines in future, why not nip them in the bud?

Consider the following diff:

- Array(sass_config.fetch("silence_deprecations", nil))
+ Array(sass_config.fetch("silence_deprecations", EMPTY_ARRAY)).

Unless sass-embedded tries to mutate the returned config value, (EMPTY_ARRAY), I don't know why we should allocate an empty array unnecessarily on each call.

All that said, I won't let this small conflict block the pull request.

@ntkme ntkme force-pushed the update-sass-embedded branch from 6187fb4 to 1e9ab9e Compare October 28, 2024 18:52
@ntkme
Copy link
Contributor Author

ntkme commented Oct 28, 2024

Unless sass-embedded tries to mutate the returned config value, (EMPTY_ARRAY), I don't know why we should allocate an empty array unnecessarily on each call.

That's actually not the fastest. It's a bit counter intuitive, that a const array is even slower than nil, probably due to cost of constant lookup. See the benchmark code and result below. I've updated the code to the fastest variation.

       user     system      total        real
[:not_an_array]			  1.358422   0.005172   1.363594 (  1.363642)
.fetch(:not_an_array, [])	  1.643419   0.007432   1.650851 (  1.651058)
.fetch(:not_an_array, ARRAY)	  1.496385   0.005461   1.501846 (  1.501929)
.fetch(:not_an_array, nil)	  1.475880   0.005275   1.481155 (  1.481180)
[:is_an_array]			  0.495788   0.001422   0.497210 (  0.497243)
.fetch(:is_an_array, [])	  0.781999   0.003789   0.785788 (  0.785809)
.fetch(:is_an_array, ARRAY)	  0.637464   0.001719   0.639183 (  0.639205)
.fetch(:is_an_array, nil)	  0.626430   0.001856   0.628286 (  0.628448)
Benchmark code
require 'benchmark'

ARRAY = []

h = {
  not_an_array: 'not an array',
  is_an_array: ['is an array']
}

n = 10000000

def a(h, k)
  Array(h[k])
end

def b(h, k)
  Array(h.fetch(k, []))
end

def c(h, k)
  Array(h.fetch(k, ARRAY))
end

def d(h, k)
  Array(h.fetch(k, nil))
end

Benchmark.bm do |x|
  # warmup
  1000.times do; a(h, :test); end
  1000.times do; b(h, :test); end
  1000.times do; c(h, :test); end
  1000.times do; d(h, :test); end

  GC.start
  x.report("[:not_an_array]\t\t\t") { n.times do; a(h, :not_an_array) ; end }
  GC.start
  x.report(".fetch(:not_an_array, [])\t") { n.times do; b(h, :not_an_array); end }
  GC.start
  x.report(".fetch(:not_an_array, ARRAY)\t") { n.times do; c(h, :not_an_array); end }
  GC.start
  x.report(".fetch(:not_an_array, nil)\t") { n.times do; d(h, :not_an_array); end }
  GC.start
  x.report("[:is_an_array]\t\t\t") { n.times do; a(h, :is_an_array) ; end }
  GC.start
  x.report(".fetch(:is_an_array, [])\t") { n.times do; b(h, :is_an_array); end }
  GC.start
  x.report(".fetch(:is_an_array, ARRAY)\t") { n.times do; c(h, :is_an_array); end }
  GC.start
  x.report(".fetch(:is_an_array, nil)\t") { n.times do; d(h, :is_an_array); end }
end

@ashmaroli ashmaroli changed the title Bump sass-embedded requirement to ~> 1.75 and add deprecation options Add support to configure more deprecation warnings from Dart Sass Oct 30, 2024
@ashmaroli
Copy link
Member

Thanks @ntkme
@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 45330f6 into jekyll:master Oct 30, 2024
5 checks passed
jekyllbot added a commit that referenced this pull request Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants