Skip to content

Remove sass-rails dependency#907

Merged
Bramjetten merged 8 commits into
SpinaCMS:masterfrom
giacope:master
Dec 10, 2021
Merged

Remove sass-rails dependency#907
Bramjetten merged 8 commits into
SpinaCMS:masterfrom
giacope:master

Conversation

@giacope
Copy link
Copy Markdown
Contributor

@giacope giacope commented Dec 10, 2021

Context

Hi all 👋

I was trying to integrate Spina into an existing project, but run into an issue when precompiling assets (everything worked fine in development). The problem is that TailwindCSS has some syntax the breaks the SassC processor:

rake aborted!
SassC::SyntaxError: Error: unterminated attribute selector for type
        on line 819:16 of stdin
>> .prose ol[type="A" s] {

Like the errors shows, this issue is certainly not Spina specific, but rather caused by Spina's sass-rails dependency. Other people have faced the same issue when including the dependency: rails/cssbundling-rails#18.

The only workaround seems to remove the sass-rails gem and I think it will be good to remove it for a few reasons:

  • Libsass is deprecated, which means sassc-rails is kinda deprecated (which is a dependency of sass-rails).
    • Rails has removed sass-rails as a default for this reason. It seems many people have faced hardships maintaining projects the rely on this gem.
    • According to this post, the lack of maintenance on libsass is what breaks when Tailwind is used due to new CSS syntax. This is also what cssbundling-rails mentions as the root cause. FWIW, Tailwind introduced the "sassc-breaking" change this year.
  • Saas has very low usage in this project and having one less dependency might be good.
  • It resolves the issue this PR aims to solve 😅 and other potentially future issues people might have due to sass-rails presence.

Changes proposed in this pull request

This PR makes the necessary changes to drop sass-rails completely. It mainly consists of file renaming and reference changes.

Guidance to review

None. All tests pass locally.

P.S: Thanks for all the work in Spina, it's an amazing project! 🤘

@giacope giacope changed the title build: remove sass Remove sass-rails dependency Dec 10, 2021
@Bramjetten
Copy link
Copy Markdown
Contributor

Thank you very much for your thorough PR! This looks great. I'm definitely in favor of dropping sass now that we're using Tailwind. Any specific reason why you chose to separate the _fonts stylesheet?

@giacope
Copy link
Copy Markdown
Contributor Author

giacope commented Dec 10, 2021

@Bramjetten Thanks for your response. No specific reason actually, I've just updated the PR to keep the same stylesheet. Also realized it actually has to use the Asset Pipeline helpers to properly compile given that the sassc importer is gone 😅

@Bramjetten
Copy link
Copy Markdown
Contributor

Bramjetten commented Dec 10, 2021

I believe it might be necessary to make the require calls in application.css explicit:

/*
 *= require spina/_animate
 *= require spina/_fonts
 *= require spina/_tailwind
 *= require_self
 */

This is necessary because Spina actually copies its own stylesheets to the main app's vendor directory in order to purge Tailwind styles after registering additional Spina plugins.

It hooks into rake assets:precompile here.

@Bramjetten Bramjetten merged commit 5b7b8f3 into SpinaCMS:master Dec 10, 2021
@Bramjetten
Copy link
Copy Markdown
Contributor

Thank you very much for your work and quick responses! 🙏

@giacope
Copy link
Copy Markdown
Contributor Author

giacope commented Dec 10, 2021

@Bramjetten Happy to! And likewise :)

@Bramjetten
Copy link
Copy Markdown
Contributor

Just pushed v2.6.1 to Rubygems 🎉

@Bramjetten
Copy link
Copy Markdown
Contributor

Not sure why, but I can't get Spina to run without sassc. It looks like Sprockets needs it (getting a LoadError on require 'sassc')

Not sure why this doesn't come up in Github Actions.

@giacope
Copy link
Copy Markdown
Contributor Author

giacope commented Dec 15, 2021

@Bramjetten Interesting. What sprockets and rails versions are you seeing the failure with? Curious if perhaps it caused by an older version prior to rails/rails#43110.

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.

2 participants