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

Update to latest sass with sass-embedded gem #148

Merged
merged 2 commits into from
Jul 3, 2024
Merged

Update to latest sass with sass-embedded gem #148

merged 2 commits into from
Jul 3, 2024

Conversation

ntkme
Copy link
Contributor

@ntkme ntkme commented Feb 23, 2024

The dartsass gem has never been updated since its very first release. This PR updates to latest version of dart sass using sass-embedded gem, which has version and feature matching release with dart-sass.

@brettchalupa
Copy link
Owner

@ntkme thank you very much for this contribution! I will test it out and review it in the coming days. 🙌

brettchalupa
brettchalupa previously approved these changes Feb 26, 2024
Copy link
Owner

@brettchalupa brettchalupa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This branch is working really well for me, thank you for this improvement! 🙌 I think the call to Sass.compile will be much more resilient than the previous implementation.

I noticed three tests are failing on main now, let me fix those. They are seemingly unrelated to these changes.

@mritunjaysharma394
Copy link

Any updates on this and if we can get this merged? I think dartsass 1.49 is not supported for arm yet The PR is merged but seems like its not released yet sass/dart-sass#2262. It seems like sass-embedded gem does support arm so probably it can help? https://rubygems.org/gems/sass-embedded/versions?page=8

Thanks for the contribution!

@ntkme
Copy link
Contributor Author

ntkme commented Jul 3, 2024

@mritunjaysharma394 You have a few misunderstandings here:

  1. The dartsass gem is a third party gem that repackages dart-sass, that it is not maintained. The only available version is massively outdated.
  2. The sass-embedded gem is also a community maintained gem that repackages dart-sass. It provides both CLI and API access. I’m the maintainer of the gem and one of the primary contributors of official dart-sass project for its embedded compiler. This gem has up-to-date, version to version matched release with dart-sass.
  3. Arm is supported by dart-sass (and sass-embedded) long time ago. The PR you’re referring about is to enable AOT (ahead-of-time) mode instead of JIT (just-in-time) mode of dart VM on linux-musl platform, which will improve the start up performance. However, it does not change anything in terms of arm platform support as it was supported long time ago.

@mritunjaysharma394
Copy link

Thanks so much @ntkme for the detailed response and explaination, that really happens to clear my confusion and I am sorry for all the confusion it caused it.

I just found out that it indeed uses thirdparty https://github.com/ayushn21/dartsass-ruby and possibly the name similarity caused the confusion. Thanks for all the help and really hope this PR still gets merged soon to use the maintained one. Thank you!

@brettchalupa
Copy link
Owner

brettchalupa commented Jul 3, 2024

An upstream change in the graphql gem broke the tests, see #149

I'd like to get that fixed so the test suite is passing before merging this. I'll work on it in the coming days and try to get this merged before the end of the weekend. Sorry for the slowness here.

If anyone is interested/willing to help get the suite green, that'd be amazing and much appreciated!

@brettchalupa brettchalupa merged commit 3a0eb90 into brettchalupa:main Jul 3, 2024
4 checks passed
@brettchalupa
Copy link
Owner

@ntkme thanks again for this contribution!

@brettchalupa
Copy link
Owner

I'm going to work on getting a new version of the gem released, likely v5.0.0 since there are breaking changes.

@ntkme ntkme deleted the update-sass branch July 3, 2024 19:10
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.

4 participants