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

Replace unmaintained dartsass-ruby with sassc-embedded #15

Merged
merged 1 commit into from
Dec 29, 2023
Merged

Replace unmaintained dartsass-ruby with sassc-embedded #15

merged 1 commit into from
Dec 29, 2023

Conversation

ntkme
Copy link
Contributor

@ntkme ntkme commented Dec 29, 2023

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 johnnyshields merged commit 94e69c7 into tablecheck:master Dec 29, 2023
1 of 62 checks passed
@johnnyshields
Copy link
Contributor

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

@johnnyshields
Copy link
Contributor

johnnyshields commented Dec 29, 2023

@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
Copy link
Contributor Author

ntkme commented Dec 29, 2023

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
Copy link
Contributor

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
Copy link
Contributor Author

ntkme commented Dec 31, 2023

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
Copy link
Contributor Author

ntkme commented Dec 31, 2023

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 tablecheck/dartsass-ruby#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
Copy link

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
Copy link
Contributor

Done — repo archived. Thanks for the suggestion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants