turbolinks / turbolinks-classic

Classic version of Turbolinks. Now deprecated in favor of Turbolinks 5.
MIT License
3.54k stars 431 forks source link

Add bower support. #602

Closed schneidmaster closed 9 years ago

schneidmaster commented 9 years ago

Backstory: A decent subset of folks (myself included) use bower to manage frontend dependencies in Rails apps. GitHub user @menglr was formerly maintaining a bower package for Turbolinks based on a fork of the main repo but they have recently deleted their GitHub account causing the package to break. I was able to get the turbolinks package name released and was wondering if it would be possible to just add bower support to the main repository rather than starting another forked repo for the package.

rails-bot commented 9 years ago

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @reed (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

javan commented 9 years ago

I'm not super familiar with bower.. How does listing a .js.coffee file work? What if that file happened to require another file with #= require ...?

schneidmaster commented 9 years ago

The main file is not super important - it is only there because it is required for a valid bower specification. It would typically point to a built/minified/zipped production .min.js file suitable for concatenation with other javascripts or whatever else. But in this case, anyone who is using Turbolinks is using Rails and anyone using Rails is just going to put it in their application.js manifest anyway, so it primarily serves as a reference.

rafaelfranca commented 9 years ago

I think I didn't understand it. Would sprockets directives like #= require works with bower?

javan commented 9 years ago

But in this case, anyone who is using Turbolinks is using Rails and anyone using Rails is just going to put it in their application.js manifest anyway, so it primarily serves as a reference.

Why is bower needed then if you're already using Rails and have Turbolinks available in your asset load path?

javan commented 9 years ago

I understand the desire to add Bower support, but it seems like we should be listing a pre-built .js file.

rafaelfranca commented 9 years ago

Also, you would have to use the gem anyway because there are a lot of Ruby code on this gem, so I don't think using bower to manage turbolinks would even work.

reed commented 9 years ago

I was just about to say what @rafaelfranca just said. Adding a pre-built .js file to the repo might mislead users into thinking that's all they need to use turbolinks.

schneidmaster commented 9 years ago

I think I didn't understand it. Would sprockets directives like #= require works with bower?

Think of it this way - bower is analogous to bundler. In my Rails project I have a bower.json that contains a list of frontend dependencies (Turbolinks, jQuery, Bootstrap, etc.), and bower handles installing all of them for me. So bower will grab the latest versions of the code for each frontend library and stick them in e.g. vendor/assets/bower_components. I then include each library from that directory in my Rails application.js and/or application.css, and Sprockets does its usual thing. Bower is essentially a replacement for copying files manually into vendor/assets in the same way that Bundler is a replacement for manually downloading a bunch of gems and sticking them in vendor/bundle. It doesn't "work with" sprockets directives because it's a dependency manager, not an asset compiler. Does that explanation make more sense?

Why is bower needed then if you're already using Rails and have Turbolinks available in your asset load path?

I use bower for a bunch of things, not just Turbolinks. jQuery, Bootstrap, etc. - essentially everything that is a frontend asset. Given that I'm using bower for everything else on the frontend, it's convenient to use it for turbolinks as well.

I understand the desire to add Bower support, but it seems like we should be listing a pre-built .js file.

Also, you would have to use the gem anyway...

I was just about to say what @rafaelfranca just said...

You do still have to use the gem, correct. Bower is just for the frontend component. Listing a pre-built JS file is not really necessary IMO since you only can use Turbolinks with Rails anyway, and might cause the confusion that @reed mentioned.

javan commented 9 years ago

I think the basic page changing behavior would work for non-Rails apps and that's a reason to register a pre-built .js file. I'm not invested in this at all so don't take this as an argument for or against. :grin:

rafaelfranca commented 9 years ago

But why do you need to use turbolinks on bower if you still needs the gem and you will still use sprockets and sprockets will load the gem's version? Just to list turbolinks as a frontend dependency?

schneidmaster commented 9 years ago

I think the basic page changing behavior would work for non-Rails apps and that's a reason to register a pre-built .js file. I'm not invested in this at all so don't take this as an argument for or against.

Ah, if true, that would actually be a great reason for a pre-built .js file as well as another reason for bower support. I was not aware that was the case.

But why do you need to use turbolinks on bower if you still needs the gem and you will still use sprockets and sprockets will load the gem's version? Just to list turbolinks as a frontend dependency?

Because I prefer to keep backend dependencies (gems) and frontend dependencies (bower packages) separated in dependency manifests, for better application structuring and so it's easy to see what is used where. It just happens that turbolinks has both backend and frontend components - but I guess I still think of it primarily as a frontend dependency.

javan commented 9 years ago

I'm -1 unless we register a usable .js file.

schneidmaster commented 9 years ago

If there's interest in that I can augment the default Rake task to build a JS file. (And update the bower.json accordingly)

rafaelfranca commented 9 years ago

Would that JS file works without the backend component?

I still don't see any good reason to support turbolinks on bower, even more because you still need the gem.

But like @javan said, if we manage to make a usable .js file that works without the backend component we can have official bower support.

reed commented 9 years ago

There are definitely things that won't work without the backend component. Disabling after non-GET requests and handling redirects, to name a couple. I'd never recommend using just the JS file.

Thibaut commented 9 years ago

I then include each library from that directory in my Rails application.js and/or application.css, and Sprockets does its usual thing.

I'm still not clear on what the benefit of using Bower is, vs. just adding #= require turbolinks at the top of your application.js. Versioning of Turbolinks is done via the gem so I don't see how using a separate manifest for the JS file makes any difference, especially since Turbolinks has no dependencies of its own. Am I missing something?

Would that JS file works without the backend component?

I don't see any major issue apart from the two small ones that @reed pointed out, which we could address by writing a server "spec". However, I'm a bit reluctant to release something we're not 100% committed to supporting over the long run, and which none of the core contributors use themselves.

If someone is up for maintaining a JS-only version of Turbolinks, I would suggest they fork the repo, write the server spec, publish on bower, get some traction, at which point we can merge and give them commit access to the main repo.

But without someone "championing" the JS version, I don't think this is a good idea. Personally I have too much on my plate to be that person.

schneidmaster commented 9 years ago

I would probably be up for championing the JS version as you described.

which we could address by writing a server "spec"

Can you elaborate on what you mean by a server "spec"?

Thibaut commented 9 years ago

Can you elaborate on what you mean by a server "spec"?

  • server must return a request_method cookie on non-GET requests, to get around this (Turbolinks automatically pops the cookie when it initializes, and the server should do that too on GET requests).
  • server must set a X-XHR-Redirected-To header on the final response of a Turbolinks request that went through one or more redirections, so that Turbolinks can reflect the final location in the browser's location bar. See this test. On the first request we store the location in the session here or here. On the next request we set the header to tell Turbolinks where we ended up.
  • Turbolinks sends an X-XHR-Referer header that is supposed to take precedence over the standard Referer header. I don't remember why we do that, except that it lets us identify Turbolinks requests for doing the previous point.

Ideally we turn that into an integration test suite.

reed commented 9 years ago

We all seem to have the same reaction to this, which is that we don't see what the benefit of it is. Simply giving the ability to require turbolinks in a bower manifest instead of application.js doesn't seem to justify the extra work required to maintain a compiled JS file (as well as the manifest itself, which we would all have to remember to update whenever we change versions).

I think it'd be best if bower support was maintained in a separate repo (like it was before that account was deleted) by someone who actually uses it. @Thibaut gave some advice on how to potentially go about that. I'm going to go ahead and close this.