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

Solve Sprockets DEPRECATION WARNING #137

Merged
merged 14 commits into from
Oct 1, 2017

Conversation

brendon
Copy link
Contributor

@brendon brendon commented May 29, 2017

I’m using Grease https://github.com/yasaichi/grease as a wrapper around
LessTemplate and ImportProcessor and have also rejigged the loading
process to make things compatible with Sprockets 2, 3 and 4.

Extra changes:

  • Turned the warnings off for now with rake to make the output of the
    tests a bit more understandable.
  • Update the README with regards to getting the test suite running
  • Fixed a couple of spec’s to look for the right engine and
    preprocessor classes
  • Removed the use of capture in run_generator. There is no
    recommended replacement.

Closes: #122 Finally! :)

Thanks to #128 for inspiration @MustafaZain.

I’m using Grease https://github.com/yasaichi/grease as a wrapper around
`LessTemplate` and `ImportProcessor` and have also rejigged the loading
process to make things compatible with Sprockets 2, 3 and 4.

Extra changes:
* Turned the warnings off for now with rake to make the output of the
tests a bit more understandable.
* Update the `README` with regards to getting the test suite running
* Fixed a couple of spec’s to look for the right engine and
preprocessor classes
* Removed the use of `capture` in `run_generator`. There is no
recommended replacement.
@brendon
Copy link
Contributor Author

brendon commented May 29, 2017

@simi, finally the test suite seems to be in order! Let me know if you need any more information about this. I think there's a large group of people hanging out for this fix :D

@tdonia
Copy link

tdonia commented May 31, 2017

this works for us (thank you, @brendon!) -- almost -- on Rails 5.0 because it eliminates Sprockets.register_engine '.less', LessTemplate in railties in this commit: brendon@f573927#diff-998c29126ad83cee99c229f1d43307b5L16

the one additional issue that we hit is the line above:

Sprockets::Engines #force autoloading errs with
block in <class:Railtie>': uninitialized constant Sprockets::Engines (NameError)

I don’t think Sprockets::Engines is required to be called explicitly to
autoload.
@brendon
Copy link
Contributor Author

brendon commented May 31, 2017

Thanks for the feedback @tdonia :) I've pushed a change that should hopefully fix that. Let me know how it works on your end. I'll see how travis likes it also.

Use `register_transformer` instead of `register_preprocessor`.
joe4dev added a commit to sealuzh/cloud-workbench that referenced this pull request Jul 11, 2017
`rails s` still yield the deprecation warning:
* See issue: metaskills/less-rails#122
* PR pending: metaskills/less-rails#137
* Workaround: `gem 'less-rails', github: 'brendon/less-rails', branch: 'fix-sprockets-loading'
@ali-anwar
Copy link

Any update on this? Thank you.

$ bundle install
$ bundle exec rake appraisal:setup
$ bundle exec rake appraisal test
$ bundle
Copy link
Collaborator

Choose a reason for hiding this comment

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

please keep bundle install here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can do that, but bundle is the same as bundle install (install is the default action). Let me know if you still want it changed back.

Choose a reason for hiding this comment

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

Anyone alive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can confirm that I am alive.

Rakefile Outdated
@@ -7,6 +7,7 @@ Rake::TestTask.new do |t|
t.libs = ['lib','test']
t.test_files = Dir.glob("test/**/*_spec.rb").sort
t.verbose = true
t.warning = false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you remove this change?

Copy link
Contributor Author

@brendon brendon Jul 26, 2017

Choose a reason for hiding this comment

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

Will do. I think it was to silence the errors from some other dependencies (therubyracer) and make the test results more rational to view.

.travis.yml Outdated
- 2.0.0
- 2.1.4
- jruby-19mode
- 2.2.7
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to drop old rubies here? Are they failing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Off the top of my head I can't remember. Depending on the rails version, we'd have to exclude certain combinations. I can add them back to the matrix and see how they test out if you like?

brendon added 3 commits July 27, 2017 09:51
Fuzzy matching doesn’t seem to work properly with RVM
This could be fixed by moving away from therubyracer in the future.
@brendon
Copy link
Contributor Author

brendon commented Jul 26, 2017

Hi @simi, I've updated the test matrix and all the tests pass. I'm only testing for the latest version of jruby. Is that ok?

I've commented out the warnings line. It's handy to have there locally to uncomment because of the excessive therubyracer warnings. We should migrate away from that perhaps in another PR since I think the Rails team did the same.

I'll wait to hear from you on the bundle vs bundle install thing.

@brendon
Copy link
Contributor Author

brendon commented Jul 31, 2017

Hi @simi, any more comments before we finalise this? :)

brendon added 2 commits August 5, 2017 21:48
Rails did this transition already. Either library will work, but we
should go with what is current. It also removes the warnings showing up
in the tests.
@drn
Copy link

drn commented Aug 25, 2017

@simi Can we get this merged in? We just upgraded sprockets / rails and are running into these warnings. Happy to help make any changes, but it looks like the changes have been made and we're just waiting on a merge + release.

@ubugnu
Copy link

ubugnu commented Sep 8, 2017

I've used the workaround:
gem 'less-rails', github: 'brendon/less-rails', branch: 'fix-sprockets-loading'
On rails 5. But less files are no longer compiled (I get a 'not found' myscript.less.css)
The same configuration works with the default gem.
Any idea on how to fix the problem?

@brendon
Copy link
Contributor Author

brendon commented Sep 10, 2017

Hi @ubugnu, thanks for reporting the error. If .less.css is a usual extension then the problem is that it wasn't added as a valid extension for Sprockets 3 that requires all extensions to be explicitly defined.

I've added the .less.css extension in there for now. Give it a try. Can others chime in on whether .less.css is a usual extension for less files in Rails?

@simi simi merged commit 2b623f6 into metaskills:master Oct 1, 2017
@brendon
Copy link
Contributor Author

brendon commented Oct 1, 2017

100,000 🎉 :D

@brendon
Copy link
Contributor Author

brendon commented Oct 1, 2017

Can you ping when the gem is released? :)

@simi
Copy link
Collaborator

simi commented Oct 1, 2017

First of all I'm really sorry for being the break at this deprecation removal problem.

I would like to thanks all involved in this, mainly @brendon for crafting this PR and @drn for friendly email ping.

This is included in fresh 3.0 release.

pierre added a commit to killbill/killbill-admin-ui that referenced this pull request Oct 2, 2017
See metaskills/less-rails#137 and
#102.

Signed-off-by: Pierre-Alexandre Meyer <pierre@mouraf.org>
@brendon brendon deleted the fix-sprockets-loading branch October 12, 2017 07:24
davidlaprade added a commit to davidlaprade/twitter-bootstrap-rails that referenced this pull request Feb 20, 2018
The warning is:

```
DEPRECATION WARNING: Sprockets method `register_engine` is deprecated.
Please register a mime type using `register_mime_type` then
use `register_compressor` or `register_transformer`.
```

This problem is really a less-rails error, and is documented in this
issue of that repo:

metaskills/less-rails#122

It was fixed with this PR, merged Oct 2017:

metaskills/less-rails#137

Bumping our less-rails dependency to 3.0.0 will give us the updates in
that PR.

Version 3.0.0 is the next version after 2.8.0, so there are very few
other changes to worry about.  You can view all of the less-rails code
changes between 2.8.0 and 3.0.0 here:

metaskills/less-rails@05186eb...7bb14e5

They are almost all test and/or README changes. So this update should be
low risk.
joe4dev added a commit to sealuzh/cloud-workbench that referenced this pull request Aug 16, 2018
`rails s` still yield the deprecation warning:
* See issue: metaskills/less-rails#122
* PR pending: metaskills/less-rails#137
* Workaround: `gem 'less-rails', github: 'brendon/less-rails', branch: 'fix-sprockets-loading'
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.

7 participants