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

Introduces support for sprockets 4.y.z releases #311

Conversation

jrgriffiniii
Copy link
Collaborator

Resolves #310 by ensuring that sprockets 4.y.z releases can be used. Also provides a solution for supporting Rails 6.0.0 releases.

@jrgriffiniii
Copy link
Collaborator Author

This remains broken given that I can't seem to understand why I'm breaking the build with engine_cart. Currently, attempting to keep webpacker from being installed with the Rails application generator involves invoking rails new --skip-webpacker-install (or, preferably, rails new --skip-javascript). Adding either to the file .engine_cart.yml does not seem to prevent webpacker from being added as a dependency, and from the generator from installing NPM packages for Webpack.

@jrgriffiniii jrgriffiniii force-pushed the issues-310-jrgriffiniii-sprockets-4 branch 2 times, most recently from 57460b1 to 6df9731 Compare November 4, 2019 14:57
when /^5\./
gem 'capybara', '~> 2.18.0'
when /^4\.2/
gem 'sass-rails', '~> 5'
when /^4\./
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This needs to be removed, Core Components are no longer required to support Rails 4.y.z releases.

@@ -34,14 +34,21 @@ else
end

case ENV['RAILS_VERSION']
when '6.0.0' # This will only be necessary until release 6.0.1 is published
gem 'sass-rails', '~> 5'
gem 'webpacker' # This is necessary or the generator will break
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This shouldn't be here, but I cannot disable engine_cart from inserting webpacker as a dependency.

spec.add_dependency 'ruby-box'
spec.add_dependency 'signet', '~> 0.8'
spec.add_dependency 'sprockets', '~> 3.7'
spec.add_dependency 'sprockets'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sprockets becomes a problem with engine_cart unless --skip-sprockets is passed to the Rails generator. This needs to be resolved in order to remove sprockets as a dependency of the Gem (at least, for Rails 6.y.z releases).

@jrgriffiniii jrgriffiniii force-pushed the issues-310-jrgriffiniii-sprockets-4 branch from 6df9731 to 1b48537 Compare November 4, 2019 15:08
@jrgriffiniii
Copy link
Collaborator Author

This provides a first step towards supporting Rails 6.0.z and Sprockets 4.y.z.

@jrgriffiniii
Copy link
Collaborator Author

From what I could determine, the modification which I need lies somewhere in https://github.com/cbeer/engine_cart/blob/master/lib/engine_cart/configuration.rb#L54. If the value of options[:rails_options] is not split on whitespace (using a method such as #split(/\s/)), then the overridden options passed in .engine_cart.yml will not parse them properly.

@jrgriffiniii
Copy link
Collaborator Author

@jrgriffiniii
Copy link
Collaborator Author

Further progress on this is blocked until cbeer/engine_cart#99 (or some guidance) directs me on how I wasn't able to generate the internal_test_app with --skip-javascript.

if Rails.version =~ /^6\./
copy_file 'app/assets/javascripts/application.js', 'app/assets/javascripts/application.js'

# Does this means that Webpacker becomes a hard-dependency?
Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, this has nothing to do with webpacker. In fact, it's the opposite -- what this line of code is doing is making sure that Sprockets will still handle Javascript, even if you are using Sprockets 4.

Since:

  • Sprockets 4 (unlike sprockets 3) will use this app/assets/config/mannifest.js file
  • Rails 6 (unlike Rails 5) will not include the link_directory ../javascripts .js line in the generated app/assets/config/manifest.js file, since Rails 6 thinks you shouldn't use Sprockets for JS.

So what you're doing is making it so Sprockets will still do JS in Rails 6.0.1 with Sprockets 4 -- nothing to do with webpacker at all.

So at least, you probably want to remove/change this comment.

But also, since this is a generator -- you probably want to document where (README?) that the browse-everything installer under Rails 6 is going to re-enable Sprockets handling of JS, and add a line to your application.html.erb layout that generates a <script> tag for a sprockets-compiled application.js in addition to the existing webpacker-compiled one.

Which is not ideal -- but is better than browse-everything not working at all, it's what we've got for Rails 6 right now.

@jrgriffiniii jrgriffiniii added this to the 1.1.0 milestone Feb 17, 2020
@jrochkind
Copy link
Contributor

jrochkind commented Apr 13, 2020

Hi I can't upgrade to browse-everything 1.0.2 (or any future b-e's), because I am already using sprockets 4. Which is working fine with browse-everything, it's not causing a problem for me. But b-e 1.0.2 no longer allows it in the gemspec

What is the status of this PR, do you think? I don't think it's okay that b-e 1.0.1 works fine with sprockets 4, but 1.0.2 will not allow a using app to use sprockets 4. I don't want to have to downgrade my sprockets to keep using b-e.

@hackartisan
Copy link
Contributor

superceded by #358

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.

Browse-everything should not require sprockets 3.x only
3 participants