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

Add missing sprockets-rails dependency #4382

Merged

Conversation

waiting-for-dev
Copy link
Contributor

@waiting-for-dev waiting-for-dev commented May 20, 2022

Rails 7 doesn't have sprockets-rails as a dependency anymore [1].
However, it's a dependency on solidus_core [2].

We hadn't caught it because our test suite depends transitively on it
through sassc-rails [3] in solidus_backend [4] & solidus_frontend [5].

However, we had an incompatibility when using standalone solidus_core
(like in the dummy app for extensions) with Rails 7.

[1] - https://guides.rubyonrails.org/7_0_release_notes.html#railties-notable-changes

[2]

require "sprockets/railtie"

[3] - https://github.com/sass/sassc-rails/blob/8d0462d54b5b5dd84cb1df31823c3afaebb64534/sassc-rails.gemspec#L31

[4]

s.add_dependency 'sassc-rails'

[5]

s.add_dependency 'sassc-rails'

Rails 7 doesn't have sprockets-rails as a dependency anymore [1].
However, it's a dependency on solidus_core [2].

We hadn't caught it because our test suite depends transitively on it
through sassc-rails [3] in solidus_backend [4] & solidus_frontend [5].

However, we had an incompatibility when using standalone solidus_core
(like in the dummy app for extensions) with Rails 7.

[1] - https://guides.rubyonrails.org/7_0_release_notes.html#railties-notable-changes
[2] - https://github.com/solidusio/solidus/blob/35d43a17124b92651ab43a36ae91f68b7f00f0c5/core/lib/spree/core.rb#L10
[3] - https://github.com/sass/sassc-rails/blob/8d0462d54b5b5dd84cb1df31823c3afaebb64534/sassc-rails.gemspec#L31
[4] - https://github.com/solidusio/solidus/blob/455754bee7a8c58489ef4b913b42c4168212929c/backend/solidus_backend.gemspec#L35
[5] - https://github.com/solidusio/solidus/blob/455754bee7a8c58489ef4b913b42c4168212929c/frontend/solidus_frontend.gemspec#L34
@kennyadsl
Copy link
Member

Thanks! Can't we try to remove this require and use it only when needed?

@waiting-for-dev
Copy link
Contributor Author

Thanks! Can't we try to remove this require and use it only when needed?

Hmmm... where would you put it? solidus_core has a manifest. Doesn't it mean that sprockets need to be required at boot time?

@kennyadsl
Copy link
Member

I forgot that we have those shared sprockets assets. They are used both in frontend and backend. To be honest I do not think it's beneficial anyhow but definitely not something that we need to address now.

@kennyadsl kennyadsl merged commit f71a764 into solidusio:master May 23, 2022
@kennyadsl kennyadsl deleted the waiting-for-dev/missing_sprockets branch May 23, 2022 07:54
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