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 for sass and sass-embedded 1.68 compatibility #13

Closed
chadlwilson opened this issue Sep 21, 2023 · 6 comments
Closed

Update for sass and sass-embedded 1.68 compatibility #13

chadlwilson opened this issue Sep 21, 2023 · 6 comments

Comments

@chadlwilson
Copy link

Looks like there is some kind of breaking change in sass 1.68

Sass::CompileError: /my-project/gems/jruby/3.1.0/gems/dartsass-ruby-3.0.1/lib/sassc/import_handler.rb:109:in `canonicalize': wrong number of arguments (given 2, expected 1) (ArgumentError)
        from /my-project/gems/jruby/3.1.0/gems/sass-embedded-1.68.0/lib/sass/embedded/host/importer_registry.rb:63:in `canonicalize'
        from /my-project/gems/jruby/3.1.0/gems/sass-embedded-1.68.0/lib/sass/embedded/host.rb:102:in `canonicalize_request'
        from org/jruby/RubyKernel.java:2119:in `public_send'
        from /my-project/gems/jruby/3.1.0/gems/sass-embedded-1.68.0/lib/sass/embedded/host.rb:148:in `await'
        from /my-project/gems/jruby/3.1.0/gems/sass-embedded-1.68.0/lib/sass/embedded/host.rb:35:in `compile_request'
        from /my-project/gems/jruby/3.1.0/gems/sass-embedded-1.68.0/lib/sass/embedded.rb:216:in `compile_string'
        from /my-project/gems/jruby/3.1.0/gems/sass-embedded-1.68.0/lib/sass/embedded.rb:53:in `compile_string'
        from /my-project/gems/jruby/3.1.0/gems/dartsass-ruby-3.0.1/lib/sassc/engine.rb:25:in `render'
        from /my-project/gems/jruby/3.1.0/gems/dartsass-sprockets-3.0.0/lib/sassc/rails/template.rb:40:in `block in call'
        from /my-project/gems/jruby/3.1.0/gems/sprockets-4.2.1/lib/sprockets/utils.rb:145:in `block in module_include'
...        

Looks like https://github.com/tablecheck/dartsass-ruby/blob/51039a20e5a05aa778baed3fc4edfcb2f6efd3cf/lib/sassc/import_handler.rb#L109 needs an update, I guess something similar to sass-contrib/sassc-embedded-shim-ruby@cb35862

(Sorry would raise the issue at dartsass-ruby but issues seem to be disabled on that repository.)

davidtaylorhq added a commit to discourse/discourse that referenced this issue Sep 26, 2023
Discourse has a custom stylesheet pipeline which compiles things 'just in time'. The only place we were still running sass files through sprockets was for the `/tests` route in development mode. This use can be removed by compiling the relevant stylesheets through ember-cli instead (which we were already doing for testem runs)

This work was prompted by the incompatibility of dartsass-sprockets with the latest sass-embedded release (tablecheck/dartsass-sprockets#13)
@gravitystorm
Copy link
Contributor

I tried working on a PR for dartsass-ruby today to fix this issue, but without success yet. Mostly because I've never looked at any of this code before. I tried making similar changes to the sassc-embedded-shim-ruby commit mentioned, but something is still not working correctly since the tests don't pass.

In the meantime, I've made a PR to dartsass-ruby to simply restrict the sass-embedded dependency to the working versions, i.e. before 1.67.0 - as a temporary workaround, but it will save a lot of hassle for users of dartsass-ruby and dartsass-sprockets until we get a proper fix.

Note that the breakage happens with 1.67.0 . The upstream dart-sass project mentions a 'potentially breaking bugfix' in the 1.67.0 changelog, which I think could be related to the root cause. Perhaps that will help someone understand what changes need to be made to dartsass-ruby.

If anyone else wants to try making a PR for dartsass-ruby that implements a proper fix, then that would be great!

@ntkme
Copy link
Contributor

ntkme commented Oct 5, 2023

Author of sass-embedded, sassc-embedded, and contributor of the official dart-sass here.

dartsass-ruby is a fork that combines the original sassc gem with my sassc-embedded shim code into a single repository. Although it is not a showing as a "fork" on GitHub, technically it is. A fun fact is that despite no commit history is tied to me in the fork, my name was added to the LICENSE to fulfill the legal requirement because it was a copy-paste.

sassc-embedded existed almost a year before tablecheck forked it, and I posted it once in sassc issue (sass/sassc-ruby#220). The reason that I did no advertisement is that I don't want to promote a stopgap solution. Someone did not get the idea, forked it, and promoted it everywhere. All of this happened with no prior commutations. After I pointed out that promoting a stopgap solution is not a good idea, the only thing happened is a "help wanted" issue (#2) with no progress at all... Ironically, despite of me pointing out many problems, I got an invite to become a maintainer of this project (sass-contrib/sass-embedded-host-ruby#97). With all the backstory so far, of course I declined. A few months pasted and now we are here.

The fixes needed are already made in sassc-embedded a while ago. However, I will not make any PR here, because I hate the counter productive entrepreneur thinking in open source. - The owner clearly does not want to maintain the project but still forked it and sold the project to the community, wishfully thinking if it could sell enough it will eventually attract maintainers. - I don't want to contribute to a project whose owner clearly has no intention to take the technical leadership or responsibility.

Finally, I have to say to the owner of this project: You forked it, you maintain it. Please be responsible.

@gravitystorm
Copy link
Contributor

@ntkme Thanks for the background information. It's not clear to me what you recommendation is for the best way forward.

Without wanting to go too far off-topic for this issue, do you agree that the proposed changes outlined in #2 are a good idea? Or is there some other approach that you would recommend instead? As far as I'm aware, there's no other gem offering integration between dartsass and sprockets, i.e. as an alternative to this one.

@ntkme
Copy link
Contributor

ntkme commented Oct 18, 2023

Yes. #2 is the right way forward if people still would like to continue use sprockets... sassc-embedded has a very complex emulation layer to make custom functions and importers behave similar to sassc. So, keep in mind that it will be a brutal breaking change when move directly to sass-embedded without emulation, because the custom functions and importers works completely differently in the new API.

@gravitystorm
Copy link
Contributor

In the meantime, I've made a PR to dartsass-ruby to simply restrict the sass-embedded dependency to the working versions, i.e. before 1.67.0 - as a temporary workaround, but it will save a lot of hassle for users of dartsass-ruby and dartsass-sprockets until we get a proper fix.

This PR has been merged, and released in dartsass-ruby 3.0.2, so hopefully various projects will start working again.

If anyone else wants to try making a PR for dartsass-ruby that implements a proper fix, then that would be great!

Just for clarity, this bit is still required 😄

@johnnyshields
Copy link
Contributor

Closing as the issue is fixed in dartsass-ruby.

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

No branches or pull requests

4 participants