hsume2 / browserify-rails

Deprecated https://github.com/browserify-rails/browserify-rails
MIT License
57 stars 11 forks source link

Not working out of the box #24

Closed rymohr closed 10 years ago

rymohr commented 10 years ago

This library looks exactly like what I've been looking for but I just can't get it to work as described. Is the README documenting the planned behavior or current?

The config settings only worked if I used github for the source instead of rubygems.

I'm also running into the same issues as @adrpac with .js.coffee files but haven't been able to find a solution https://github.com/hsume2/browserify-rails/issues/22

rymohr commented 10 years ago

To get things working I ended up having to change the dependencies line from

run_browserify("--list")

to

run_browserify("#{options} --list")
rymohr commented 10 years ago

That's in addition to having to add the following command line options

# application.rb
config.browserify_rails.commandline_options = "-t coffeeify --extension=\".js.coffee\""
rymohr commented 10 years ago

Are we doing something wrong or should I put this into a pull request?

martenlienen commented 10 years ago
  1. The version on rubygems should be current
  2. You should not have to add options for coffeescript, because browserify is supposed to run after previous compilation steps have been invoked. Can you check, if data in run_browserify returns javascript or coffeescript code?
martenlienen commented 10 years ago

I have just pushed a test for browserifying a coffee file and everything seems to work as expected. Could you please compare your setup with the test?

rymohr commented 10 years ago

The readme instructs

gem "hsume2-browserify-rails", "~> 0.3", :require => "browserify-rails"

but 0.3 is only available on the browserify-rails gem, not the hsume2-browserify-rails. Should we be using that one instead?

https://rubygems.org/gems/browserify-rails https://rubygems.org/gems/hsume2-browserify-rails

rymohr commented 10 years ago

I think the issue has to do with mixing sprockets #= require directives and commonjs require(...) in a single coffeescript file. Haven't nailed it down yet though.

martenlienen commented 10 years ago

This should not be an issue, because the sprockets directives are already fully resolved when browserify is called. I have these mixed, too, in a project. It is only javascript though, so no coffeescript involved.

rymohr commented 10 years ago

Yeah, you would only run into this with coffeescript. It looks like this library relies on the asset pipeline to preprocess coffeescript sources but the run_browserify("--list") is outside the pipeline.

# @return [<String>] Paths of files, that this file depends on
def dependencies
  @dependencies ||= run_browserify("--list").lines.map(&:strip).select do |path|
    # Filter the temp file, where browserify caches the input stream
    File.exists?(path)
  end
end

See the failing test I added at https://github.com/rymohr/browserify-rails/blob/24-coffeescript-busted/test/compilation_test.rb#L84

martenlienen commented 10 years ago

Hm, run_browserify passes the already processed file content (data) directly to browserify via stdin to circumvent this exact problem. Could you further investigate what exactly the problem is?

rymohr commented 10 years ago

The current file has already been processed but I don't think that's true for the require() dependencies in this case.

martenlienen commented 10 years ago

What is the exact exception then?

rymohr commented 10 years ago
throw Error(\"BrowserifyRails::BrowserifyError: Error while running `~/dev/browserify-rails/test/dummy/./node_modules/.bin/browserify  --list -`:

events.js:72
        throw er; // Unhandled 'error' event
              ^
Error: module "./mocha" not found from "~/dev/browserify-rails/test/dummy/app/assets/javascripts/fake_9a14596c.js"
    at notFound (~/dev/browserify-rails/test/dummy/node_modules/browserify/index.js:811:15)
    at ~/dev/browserify-rails/test/dummy/node_modules/browserify/index.js:761:23
    at ~/dev/browserify-rails/test/dummy/node_modules/browserify/node_modules/browser-resolve/index.js:185:24
    at ~/dev/browserify-rails/test/dummy/node_modules/browserify/node_modules/resolve/lib/async.js:36:22
    at load (~/dev/browserify-rails/test/dummy/node_modules/browserify/node_modules/resolve/lib/async.js:54:43)
    at ~/dev/browserify-rails/test/dummy/node_modules/browserify/node_modules/resolve/lib/async.js:60:22
    at ~/dev/browserify-rails/test/dummy/node_modules/browserify/node_modules/resolve/lib/async.js:16:47
    at Object.oncomplete (fs.js:107:15)

  (in ~/dev/browserify-rails/test/dummy/app/assets/javascripts/sprockets.js.coffee)\")

Failing test is available in https://github.com/rymohr/browserify-rails/blob/24-coffeescript-busted

rymohr commented 10 years ago

Adding coffeeify and changing

run_browserify("--list")

to

run_browserify("-t coffeeify --extension=\".js.coffee\" --list")

fixes the error.

martenlienen commented 10 years ago

I looked at this and the error is not coming from sprockets but from rails' naming scheme and the fact, that coffee script is used. So the test still fails, when you remove the sprockets statement. First the required module is not found, because browserify is looking for a .js file, but it is in a .js.coffee file. That is fixed with the --extension option. Second the found file is in coffeescript, which raises an error, because browserify tries to parse it recursively, but fails with a syntaxerror.

rymohr commented 10 years ago

Nice catch on not needing the sprockets directive. I thought I had tried that but I just confirmed it'll fail without it.

So in summary the other coffeescript tests are passing because they only require plain .js files?

The change I made here gets you passed this error as long as you add coffeeify as a dependency and update the command line options:

config.browserify_rails.commandline_options = "-t coffeeify --extension=\".js.coffee\""
greghuc commented 10 years ago

@rymohr are you planning on submitting a CR for the #{options} addition?: https://github.com/rymohr/browserify-rails/commit/924d78fbdcc5625dca2a9ec78d2c463dda5ea641

I just ran into the exact same issue as you, and the changes in the previous comment got coffeescript working for me.

Thanks for your hard work!

rymohr commented 10 years ago

I was waiting to hear @CQQL's thoughts on the proper fix. There may be another way to generate the dependencies that sidesteps this issue.

I'll keep my fork open until the issue is resolved though so feel free to use it within your Gemfile

gem 'browserify-rails', :git => 'https://github.com/rymohr/browserify-rails.git'
martenlienen commented 10 years ago

Adding the options to the dependencies call seems to be our best bet, as I would not like to hardcode support for any specific cross-compiled language.

@rymohr Can you wrap your change into a PR and also update the section about coffeescript in the readme? And please update your test case, so that it only tests requireing .js.coffee files (so remove the sprockets #require). Thanks

rymohr commented 10 years ago

Agreed. I'll throw the PR together.

rymohr commented 10 years ago

See https://github.com/hsume2/browserify-rails/pull/26

martenlienen commented 10 years ago

Merged and pushed version 0.3.1 to rubygems.