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

Add an option to skip installation of Turbolinks #15716

Merged
merged 1 commit into from
Jun 17, 2014

Conversation

schuetzm
Copy link
Contributor

There are already options for disabling Spring, Sprockets, ActiveRecord, Javascript in general, and several other components. This PR adds one for skipping Turbolinks.

@schuetzm schuetzm changed the title Add an option to skip installation of Turblinks. Add an option to skip installation of Turbolinks Jun 14, 2014
@sgrif
Copy link
Contributor

sgrif commented Jun 14, 2014

Do we need a test that turbolinks is included by default?

@rafaelfranca
Copy link
Member

This was submitted several times and we rejected. You can easily remove
turbolinks from you gemfile. Also you can use generator templates to remove
it. We are not going to add a new option for every entry on the gemfile.
On Jun 14, 2014 8:48 AM, "Sean Griffin" notifications@github.com wrote:

Do we need a test that turbolinks is included by default?


Reply to this email directly or view it on GitHub
#15716 (comment).

@senny
Copy link
Member

senny commented Jun 14, 2014

Agree with @rafaelfranca, there's not much to skip here. Just remove the entry and bundle. You'll be good to go.

@schuetzm thank you for your work.

@senny senny closed this Jun 14, 2014
@schuetzm
Copy link
Contributor Author

@senny That's not enough. You also need to remove the line in the JS manifest. And there are the data-turbolinks attributes in the default layout file. And who knows where else there are traces of it... (Well, I do now know there are no other places, but an average user won't. I always had to look it up.)

@senny
Copy link
Member

senny commented Jun 14, 2014

@schuetzm I can reopen but as @rafaelfranca said, it was submitted before and was rejected for the reasons presented. Let's hear some more opinions.

@senny senny reopened this Jun 14, 2014
@aditya-kapoor
Copy link
Contributor

Actually I am also in support for this 👍. We have options for skipping Spring, Sprockets, ActiveRecord, Javascript etc. because they are the defined defaults for the Rails app. Since Turbolinks is now included by default, I think we should have an option for disable that too..

@matthewd
Copy link
Member

If all we must do is remove the GemfileEntry, it seems there's an opportunity to add a single option for every entry in the gemfile: --skip-gems=turbolinks,coffee-rails. (I'm thinking entirely unconstrained: if you try to --skip-gems=rails, it will presumably fail in some undefined manner, and that's okay.)

I'm gently in favour of some way to do this: turbolinks is the one part of the default stack I'll generally remove... though I'd normally wait until it seems to be interfering.

@rafaelfranca
Copy link
Member

So, we already have a way to remove entirely turbolinks using generators
template. I think it is not documented though.
On Jun 14, 2014 4:16 PM, "Matthew Draper" notifications@github.com wrote:

If all we must do is remove the GemfileEntry, it seems there's an
opportunity to add a single option for every entry in the gemfile:
--skip-gems=turbolinks,coffee-rails. (I'm thinking entirely
unconstrained: if you try to --skip-gems=rails, it will presumably fail
in some undefined manner, and that's okay.)

I'm gently in favour of some way to do this: turbolinks is the one part
of the default stack I'll generally remove... though I'd normally wait
until it seems to be interfering.


Reply to this email directly or view it on GitHub
#15716 (comment).

@schuetzm
Copy link
Contributor Author

@matthewd I didn't write this because I thought I might be the only one, but I also always end up removing it. I tried to work with it several times, but it always gets in the way sooner or later.

@rafaelfranca Is this easy to use? Because I suspect if people first have to put together a generator template, it will be worse than the current situation...

@philnash
Copy link
Contributor

I support the idea of an option to remove turbolinks, I always remove it from my apps.

I just pull requested (#15766) an update to the generated application layout to move the JavaScript from the head to the bottom of the body. Whilst that doesn't make sense in light of including turbolinks, I'd like an easy to way to exclude turbolinks and have the layout generated with the JavaScript at the bottom of the body and this seems to be a good start towards that.

@rafaelfranca
Copy link
Member

@schuetzm it is very easy, just create a new ruby file, and put this content:

add_gem_entry_filter { |gem|
  gem.name != 'turbolinks'
}

You can even place it on ~/.railsrc and use as default to all new applications.

@schuetzm
Copy link
Contributor Author

As I feared, that's more complicated than removing it manually. I doubt most users will know how to do that without looking it up, and then we're again where we are now.

@rafaelfranca
Copy link
Member

Like I said it is not documented but I don't think it is much more complicated than --skip-turbolinks. I still 👎 for adding a new option to skip turbolinks or any other gem since the number of options in the future will only increase.

@rafaelfranca
Copy link
Member

I'll merge this but I'm going to implement a better API to avoid adding new options to every entry in our Gemfile. Thanks

rafaelfranca added a commit that referenced this pull request Jun 17, 2014
Add an option to skip installation of Turbolinks
@rafaelfranca rafaelfranca merged commit 0724706 into rails:master Jun 17, 2014
@rafaelfranca
Copy link
Member

Implemented at 1056589 Thanks for the test case

@schuetzm schuetzm deleted the skip-turbolinks branch June 18, 2014 08:35
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.

None yet

7 participants