search

twbs / bootstrap-rubygem

Bootstrap rubygem for Rails / Sprockets / Hanami / etc
https://rubygems.org/gems/bootstrap
MIT License
2.02k stars 447 forks source link

Soft-dependency to Sass engine to allow usage of SassC or Dart Sass #256

Closed SebouChu closed 1 year ago

SebouChu commented 1 year ago

Fix #255

Removed the hard dependency to sassc-rails to allow the usage of dartsass-sprockets in newer applications, SassC being deprecated.

jcoyne commented 1 year ago

Sprockets is also soft-deprecated and propshaft is being pushed at the replacement. Does it make sense to continue with sprockets here? Why not dartsass-rails?

glebm commented 1 year ago

There is no need for asset gems in the post-Sprockets world, right (node-based setup or https://github.com/rails/cssbundling-rails)? It's compatible with the npm ecosystem directly?

@SebouChu Looks like CI needs to be upgraded as well because of the Ruby version bump

jcoyne commented 1 year ago

@glebm yes and no. You can use cssbundling-rails and use bootstrap's scss from the npm package. However, then you need a JS runtime.

dartsass-rails promises to use dartsass to compile sass provided in gems (like this one) without a dependency on javascript or npm.

gravitystorm commented 1 year ago

@SebouChu Do you have a chance to update this PR with the requested changes to CI (i.e. drop the unsupported ruby 2.5 and rails 4.2 from the test matrix)? If not, I'm happy to pick up the baton on this.

SebouChu commented 1 year ago

@gravitystorm I'll handle this ASAP! Thanks!

gravitystorm commented 1 year ago

@SebouChu thanks for the updated PR!

I'm not a maintainer here, so feel free to ignore me. But I would consider rebasing (e.g. with git rebase -i so that you avoid having "fixup" commits and avoid repeated merging of main into your branch. Ideally a PR would just include the commits required to implement your proposal.

SebouChu commented 1 year ago

Oh okay ! I'll do that !

SebouChu commented 1 year ago

@gravitystorm I think I managed to do what you asked!

gravitystorm commented 1 year ago

Thanks @SebouChu , looks good to me.

gravitystorm commented 1 year ago

If a maintainer could please enable the workflows for this PR, that would be helpful!

glebm commented 1 year ago

Enabled

SebouChu commented 1 year ago

@glebm @gravitystorm Okay! Based on what we said, I removed any hard dependency to a Sass engine, added instructions in the README and require instructions with fallback.

I also removed the whole "dropping older Ruby and Rails versions", given that people can still use sassc-rails, which makes the gem upgrade less "breaking change"-style.

Tell me if it's fine for you!

glebm commented 1 year ago

Looks good, thank you!

SebouChu commented 1 year ago

Thank you @glebm!

gravitystorm commented 1 year ago

Thanks everyone! I would be great to have this in a released gem version, to smooth the upgrade path for everyone.

glebm commented 1 year ago

I've updating to v5.3.2 on main but a couple of tests are failing. @SebouChu can you please take a look?

gravitystorm commented 1 year ago

That will be https://github.com/tablecheck/dartsass-sprockets/issues/13 that's now causing those failures. In short, a new version of https://github.com/ntkme/sassc-embedded-shim-ruby has been released since the last time CI ran in this repository, and that introduced a breaking change which dartsass-sprockets has not yet dealt with.

gravitystorm commented 1 year ago

That will be tablecheck/dartsass-sprockets#13 that's now causing those failures.

dartsass-ruby 3.0.2 was released earlier today, which should fix the problems here in CI.

glebm commented 1 year ago

Yep, fixed!