search

sass / sassc-ruby

Use libsass with Ruby!
MIT License
363 stars 157 forks source link

Replace Libsass dependency with sass-embedded #240

Open johnnyshields opened 2 years ago

johnnyshields commented 2 years ago

Combine Natsuki's sassc-embedded-shim-ruby gem with sassc-ruby

TODO:

johnnyshields commented 2 years ago

I've tested this and its working with Rails .scss files.

ashkulz commented 2 years ago

@johnnyshields can you update the PR to allow sassc-embedded 1.55 and above? That version includes native gems, which will make deployment quite easy.

xfalcox commented 1 year ago

Great work!!! We are super happy to see this as we've been discussing what to do regarding SassC in @discourse for a long time.

This appears to be a drop-in replacement in most cases, but during my tests this appears to be more strict about source maps URLs and files, making quite a few specs fail.

From a cursory look, this appears to be coming from new Ruby code, and not from dart-sass, so can this behavior be behind a knob of some sorts?

johnnyshields commented 1 year ago

@xfalcox given the complexity/magnitude of the change it will be much easier/cleaner to abandon SassC altogether and release this as a new major version. Maintenance releases (if any) for SassC can happen on the old major version branch.

I have been running this branch in production without issues at TableCheck for several months. Let me know what steps I can take to get this PR merged.

xfalcox commented 1 year ago

@xfalcox given the complexity/magnitude of the change it will be much easier/cleaner to abandon SassC altogether and release this as a new major version. Maintenance releases (if any) for SassC can happen on the old major version branch.

Yeah sure, 100% agree.

What I meant is that I tried doing the upgrade to use this branch, and it works just fine, except for the call at

https://github.com/tablecheck/sassc-ruby/blob/embedded-sass-merge/lib/sassc/url.rb#L35

started by

https://github.com/tablecheck/sassc-ruby/blob/embedded-sass-merge/lib/sassc/engine.rb#L133

which tried to find a valid file path in the source map, which I don't think it's mandatory for the upgrade.

Can we please, if possible, put this path file validation behind a setting so we can disable and make the move to this new version easier?

johnnyshields commented 1 year ago

@xfalcox that sounds fine, can you raise a PR to my branch and I'll merge it?

johnnyshields commented 1 year ago

@ashkulz

can you update the PR to allow sassc-embedded 1.55 and above?

The gemspec currently includes spec.add_runtime_dependency 'sass-embedded', '~> 1.54'. I'd like to leave this as-is as this is what I've tested. It will allow newer versions in the 1.x series, so 1.55 is fine.

ashkulz commented 1 year ago

@johnnyshields thanks, we're using it successfully in Production. I forgot to update my comment :see_no_evil:

johnnyshields commented 1 year ago

I've also been using it in production for a few months on a large app with lots of scss files with no issue.

xfalcox commented 1 year ago

@xfalcox that sounds fine, can you raise a PR to my branch and I'll merge it?

Opened https://github.com/tablecheck/sassc-ruby/pull/1. With it I can get all specs to pass in @discourse with minimal changes and then work towards being able to re-enable the source map file validation over time.

johnnyshields commented 1 year ago

@xfalcox looks good, just merged it. I appreciate if anyone can help get this PR merged and officially released on this gem.

xfalcox commented 1 year ago

@xfalcox looks good, just merged it. I appreciate if anyone can help get this PR merged and officially released on this gem.

Sent another one with a oneliner fix from my previous PR.

johnnyshields commented 1 year ago

@xfalcox merged

xfalcox commented 1 year ago

This is looking great at @discourse. We are just waiting on this PR to land and a new gem to be released with it.

johnnyshields commented 1 year ago

Who can merge this PR?

dentarg commented 1 year ago

I suspect that would be @bolandrm, at least that is the only person who currently can make a release, as the sole gem owner at RubyGems.org: https://rubygems.org/gems/sassc

ahorek commented 1 year ago

SassC means Sass in C (libsass). In my opinion, you should keep this gem as is and release the dart implementation as a new gem based on this PR and name it appropriately. it's cleaner this way and you can maintain the new gem on your own, but the downside is that dependent gems like https://github.com/rails/sprockets have to be updated as well. It shouldn't be too much work. What do you think?

javierjulio commented 1 year ago

No, I think it's more gems that would have to be updated to support this. Not sure if something in sprockets has to change but it would in sassc-rails and also sass-rails. More functionality from those gems would have to be duplicated if a whole new gem were to be created to replace that tree. I do think it makes sense to make this change here as a new major release in this gem as a stop gap.

ahorek commented 1 year ago

that's what happened with the original https://github.com/sass/ruby-sass gem

sprockets still supports both separated gems on the Rails side, sass was replaced by sassc, see https://github.com/rails/sass-rails/pull/424

I agree it's easier to keep it in this repo instead of duplicating the API to a new fork and forcing users to change the gem, but I don't believe the original and only maintainer will ever merge it, since he's been inactive for more than 2 years...

xfalcox commented 1 year ago

I'm ready to start using this in production in @discourse. @bolandrm is there any change a major release of the gem can be cut using this? Should we give up on that and look into forking?

johnnyshields commented 1 year ago

dartsass-ruby gem is released! 🎉

johnnyshields commented 1 year ago

dartsass-sprockets gem also released. Now time to test it!

johnnyshields commented 1 year ago

@xfalcox looks like its working. You can replace sassc-rails with dartsass-sprockets in your Gemfile.

johnnyshields commented 1 year ago

@xfalcox by the way, it looks like you broke a few tests related to sourcemaps. Please kindly take a look here: https://github.com/tablecheck/dartsass-ruby (sorry CI isn't running yet.)

javierjulio commented 1 year ago

@johnnyshields thank you! Yesterday, I gave this a try locally and in our GHA CI. So far its worked great in development. Removing sassc has solved our primary issue with Docker builds as installing the gem alone would take ~4 minutes.

Is it possible using either dartsass-ruby or dartsass-sprockets to specify a dart-sass quietDeps flag to silence deprecation warnings? We have many due to foundation sites and emails.

I've noticed some broken links in the repo and on the gemspec from Rubygems so I'll create PRs for that.

javierjulio commented 1 year ago

@johnnyshields can you please remove the fork relationship on both gems? You should be able to do this through GitHub chatbot https://stackoverflow.com/a/16052845 now. The reason is that I can't create a fork of dartsass-sprockets since it forks sassc-rails instead which already exists on my account.

johnnyshields commented 1 year ago

@javierjulio thanks for the chatbot link, I've raised the ticket for both gems just now.