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 Turbolinks 5 Support Version Check #270

Merged

Conversation

aaronvb
Copy link
Member

@aaronvb aaronvb commented Feb 11, 2016

  • Add version check by using feature detection.
  • Update dummy app to use Turbolinks 5

Alternate method to #264

Review on Reviewable

@aaronvb aaronvb force-pushed the turbolinks_5_support_version_check branch 2 times, most recently from 3855aac to 1b3cd0b Compare February 11, 2016 20:48
@justin808
Copy link
Member

@aaronvb this looks great

Couple things:

  1. Update the

https://github.com/shakacode/react_on_rails/blob/master/docs%2Fadditional_reading%2Fturbolinks.md

with some info on this.

  1. Can we have travis run both? I'm pretty sure we can with 2 env setups (@dylangrafmyre)

I'd like to get this into v3. I'll merge tomorrow once I hear back.

@aaronvb
Copy link
Member Author

aaronvb commented Feb 12, 2016

@justin808 I couldn't get rspec to recognize two different versions of turbolinks because it seems to always default back to the turbolinks in the root Gemfile.

I tried using an ENV variable wrapped around the gem include and having a new rake task invoke it, but that didn't work.

if ENV["DISABLE_TURBOLINKS"].nil? || ENV["DISABLE_TURBOLINKS"].strip.empty?
  if ENV["TURBOLINKS_5"].nil? || ENV["TURBOLINKS_5"].strip.empty?
    gem 'turbolinks', '2.5.3'
  else
   gem 'turbolinks', '~> 5.0.0.beta' 
  end
end

I also tried setting up a 2nd dummy app with turbolinks 5 in the gemfile and it would still install the turbolinks from the root Gemfile. I know @robwise wrote something about the gems in the root Gemfile being used for the dummy app and travis-ci, maybe he can shed some light here.

@aaronvb
Copy link
Member Author

aaronvb commented Feb 12, 2016

Update: I may have found a way test both versions of Turbolinks. Running on CI right now.

@aaronvb aaronvb force-pushed the turbolinks_5_support_version_check branch from 1b3cd0b to 1ec6161 Compare February 13, 2016 02:06
@@ -26,6 +26,7 @@ install:
- npm install -g npm
- npm install -g poltergeist
- bundle install
- ENABLE_TURBOLINKS_5=TRUE bundle install
Copy link
Member Author

Choose a reason for hiding this comment

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

Any gems that are used in spec/dummy need to be installed before running the tests because Travis-CI will look for the source here when running bundle install in the spec/dummy folder. This basically tells travis-ci to install turbolinks classic and turbolinks 5.

@aaronvb
Copy link
Member Author

aaronvb commented Feb 13, 2016

Working on doc update.

@aaronvb aaronvb force-pushed the turbolinks_5_support_version_check branch from 1ec6161 to 9e69ac4 Compare February 13, 2016 02:29
- Add version check by using feature detection.
- Add a new rake task that runs the dummy app with Turbolinks 5
- Update travis config to install Turbolinks 5
- Use explicit turbolinks classic version(2.5.3)
- Update turbolinks and contributing doc
@aaronvb aaronvb force-pushed the turbolinks_5_support_version_check branch from 9e69ac4 to fd2c55f Compare February 13, 2016 05:49
@justin808
Copy link
Member

:lgtm_strong:

👏 👏 👏 👏 👏 👏 👏


Reviewed 7 of 8 files at r1, 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


Comments from the review on Reviewable.io

justin808 added a commit that referenced this pull request Feb 14, 2016
@justin808 justin808 merged commit 924a223 into shakacode:master Feb 14, 2016
@justin808 justin808 mentioned this pull request Feb 14, 2016
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.

3 participants