jwhitley / requirejs-rails

RequireJS support for your Rails 3 or 4 application
MIT License
592 stars 202 forks source link

Sprockets 3 support #217

Closed koenpunt closed 9 years ago

koenpunt commented 9 years ago

With Sprockets 3 around the corner, here is an initial setup.

Comments appreciated

carsomyr commented 9 years ago

@koenpunt Can the MIME type detection be done with just the hash lookup?

carsomyr commented 9 years ago

Reported in #219.

koenpunt commented 9 years ago

I'm not sure if the digest_path being used, returns the digest for the combined file (what should be), or that of the original.

But I'm using this patch without problems thusfar.

koenpunt commented 9 years ago

@koenpunt Can the MIME type detection be done with just the hash lookup?

I'm not sure what you mean by this, are you asking if the hash lookup is backwards compatible? Then no. Or that the we don't want to be backwards compatible and the conditional shouldn't be there at all?

jankeesvw commented 9 years ago

Hmm, i was just trying to deploy but that's impossible:

/usr/local/bin/ruby /var/deploy/timiapp.com/web_head/shared/bundle/ruby/2.1.0/bin/rake requirejs:precompile:all RAILS_ENV=staging RAILS_GROUPS=assets
rake aborted!
NoMethodError: undefined method `extension_for_mime_type' for #
/var/deploy/dev.timiapp.com/web_head/shared/bundle/ruby/2.1.0/gems/requirejs-rails-0.9.5/lib/tasks/requirejs-rails_tasks.rake:93:in `block (3 levels) in '
Tasks: TOP => requirejs:precompile:all => requirejs:precompile:prepare_source
(See full trace by running task with --trace)
rake aborted!
/var/deploy/dev.timiapp.com/web_head/shared/bundle/ruby/2.1.0/gems/requirejs-rails-0.9.5/lib/tasks/requirejs-rails_tasks.rake:19:in `ruby_rake_task'
/var/deploy/dev.timiapp.com/web_head/shared/bundle/ruby/2.1.0/gems/requirejs-rails-0.9.5/lib/tasks/requirejs-rails_tasks.rake:86:in `block (3 levels) in '
Tasks: TOP => assets:precompile => requirejs:precompile:external
(See full trace by running task with --trace)
koenpunt commented 9 years ago

@jankeesvw The current version of requirejs-rails doesn't work with Sprockets 3

jankeesvw commented 9 years ago

@koenpunt is there a way to fix this?

Do you know what the last version of Rails is with Sprockets 2? So I can downgrade rails in the meantime...?

koenpunt commented 9 years ago

Latest version for Rails I don't know, but you can try setting the version for sprockets explicitly to 2.0:

gem 'sprockets', '~> 2.0'

Or you can choose to use my branch by specifying it in your Gemfile:

gem 'requirejs-rails', github: 'koenpunt/requirejs-rails', ref: 'sprockets-3'
jankeesvw commented 9 years ago

@koenpunt I'm using your fork to do a deployment of our app https://timiapp.com and it works perfectly.

koenpunt commented 9 years ago

I'm running my fork on a closed environment for a month or so, without problems.

jankeesvw commented 9 years ago

@jwhitley is there a reason you are not merging this PR?

jwhitley commented 9 years ago

@jankeesvw because @carsomyr (to whom I'm ever thankful) is the active maintainer of requirejs-rails. I mostly just lurk up in the URL bar. It seems his attention got pulled away from this issue.

To help matters along, it's not been made 100% clear in this thread whether this PR would cause a regression for Sprockets 2.x users. Merging this PR without clarifying that version dependency would simply compound the problem.

I'll observe that Rails release has an unversioned dependency on sprockets-rails. This means a Rails release will currently always pull the latest Sprockets, including the recent major version bump of sprockets in the sprockets-rails gemspec. IMNSHO, this seems a questionable policy. But I'm in the midst of dragging a mature Rails app kicking, screaming, and biting from Rails 2.3 to Rails 4.2. I may be a touch biased right now.

carsomyr commented 9 years ago

@koenpunt @jankeesvw @jwhitley I've been working off of a private branch, trying to clean up a bunch of code. Perhaps that was a tad ambitious. Will release a new version tonight with just the PR.

carsomyr commented 9 years ago

@koenpunt @jankeesvw Now I remember the difficulty with merging this PR: It dealt with the removal of Bower asset detection. I'll have to test this, unless you can provide a justification as to why it can be removed.

koenpunt commented 9 years ago

Bower is integrated into Sprockets, so no problems there. Also I'm still not sure about: https://github.com/jwhitley/requirejs-rails/pull/217#issuecomment-94054054

koenpunt commented 9 years ago

also, I didn't test this with older versions of rails, but if BC isn't an issue, then

+      js_ext = if requirejs.env.respond_to?(:extension_for_mime_type)
+        requirejs.env.extension_for_mime_type("application/javascript")
+      else
+        requirejs.env.mime_types["application/javascript"][:extensions].first
+      end

can simply be:

+      js_ext = requirejs.env.mime_types["application/javascript"][:extensions].first
carsomyr commented 9 years ago

@koenpunt You can't just simply remove the Bower support, because requirejs-rails doesn't know the presence of bower.json means that something like, say, ember can be materialized by requesting the ember.js script.

koenpunt commented 9 years ago

Like I said, Bower support is integrated into sprockets. But there is probably a call to Sprockets.environment.resolve missing, investigating this atm.

carsomyr commented 9 years ago

@koenpunt I'd love to take that piece of code out. Tell me how you intend to. Remember that RequireJS needs its compilation units to be staged into a directory first.

koenpunt commented 9 years ago

@carsomyr just to be sure; if one for example has underscore in its bower.json, it should be possible to reference it from requirejs by just using underscore? Or do we expect users to a entry to paths in requirejs.yml for underscore: 'underscore/underscore'?

carsomyr commented 9 years ago

@koenpunt That's the intent: To be able to reference just underscore with RequireJS, precisely because Sprockets supports Bower, per your earlier point.