search

tablecheck / dartsass-sprockets

Integrate Dart Sass with Sprockets (Ruby on Rails asset pipeline)
MIT License
35 stars 5 forks source link

Replace unmaintained dartsass-ruby with sassc-embedded #15

Closed ntkme closed 8 months ago

ntkme commented 8 months ago

The dartsass-ruby is not properly maintained that it does not support latest sass-embedded, and it's missing lots of bug fixes from sassc-embedded.

One of the major stakeholder discourse has already moved from dartsass-ruby to sassc-embedded for this reason. So, for the sake of everyone, it's better to just get rid of dartsass-ruby and use sassc-embedded instead.

@johnnyshields I guess the reason you "forked" sassc-embedded is because you don't want to install patched sassc from git, now that is no longer necessary. So I hope you can merged this and archive the dartsass-ruby repository.

johnnyshields commented 8 months ago

@ntkme thank you so much for this! Merged and I will release a new version soon and sunset the dartsass-ruby gem.

johnnyshields commented 8 months ago

@ntkme I've looked at your recent changes for sassc-embedded. It is certainly a welcome and positive step that it's no longer required to install a forked Github repo (as you noted above, this was a deal-breaker previously.)

However, I see that the sassc gem is now in the vendor directory, and still you have the "shim" code which monkey-patches it. I hope you will consider consolidating this in a future version (I did such consolidation previously in dartsass-ruby.)

ntkme commented 8 months ago

However, I see that the sassc gem is now in the vendor directory, and still you have the "shim" code which monkey-patches it. I hope you will consider consolidating this in a future version (I did such consolidation previously in dartsass-ruby.)

For end users it makes almost no difference as the libsass native extension has been removed from the bundled original sassc. It is this way to remind everyone that regardless of how it is packaged, it is nothing but a shim to provide API compatibility with sassc. Performance cost of loading "patch" is negligible, therefore I don't see much value in changing the structure. Development wise, it is separate so that I can see what the original code is, to avoid any accidental function signature change that would diverge from original sassc.

johnnyshields commented 8 months ago

The tests are good enough to verify the original sassc function. In fact, there is quite a bit of code which can be safely removed. sassc isn't coming back, so time for it to evolve and move forward.

ntkme commented 8 months ago

so time for it to evolve and move forward.

Yes, I agree. I would suggest take another look at #2. Don’t worry about how that shim is going to evolve because it will not evolve anymore other than receiving bug fixes. New features only get added to sass-embedded, and will not be ported to the shim.

ntkme commented 8 months ago

Let me be clear: Even with this PR, I'm not endorsing sassc-embedded as a good long term solution. The only reason I make this PR is because dartsass-ruby is effectively not maintained and falling behind way too much.

Please keep in mind it is not just falling behind on the shim code of sassc-embedded. Because of this change https://github.com/tablecheck/dartsass-ruby/pull/6, sass-embedded is locked to an old version and so does the bundled dart-sass binary, meaning the following set is how much it was truly behind:

I like that you want to evolve and move forward, but what you were doing was backwards, and it caused everyone to be thousands of lines (or, hundreds of lines ignoring some non-code changes) behind. This is the only reason I made this PR.

chadlwilson commented 8 months ago

Thank you both for making this happen. I appreciate it!

Managed to switch a (sadly sprockets-dependent) project from sassc-rails and the weird sassc fork/PR to this gem, which feels much better (and is transparently cacheable etc)

Just a reminder/suggestion that the whole repo at https://github.com/tablecheck/dartsass-ruby might be best marked as archived so it gets the big ugly Github banner that will encourage folks to check the README :-)

johnnyshields commented 8 months ago

Done — repo archived. Thanks for the suggestion.