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

Compatibility with Ruby 2.0+ #651

Merged
merged 6 commits into from
Dec 26, 2016
Merged

Compatibility with Ruby 2.0+ #651

merged 6 commits into from
Dec 26, 2016

Conversation

bbonamin
Copy link
Contributor

@bbonamin bbonamin commented Dec 20, 2016

First of all, let me express how grateful I am for this gem. I've recently been learning about react and webpack, and react_on_rails has made it a breeze to implement what I've learned in my Rails projects! It's allowed us to add "sprinkles" of React in certain Rails views, without commiting to rewriting everything from scratch. Thank you!

Now, for a project I'm currently working on, we need compatibility with Ruby 2.0 (yeah yeah, I know it's not receiving fixes or patches anymore, but we can't upgrade yet.)

The rubygems page for react_on_rails doesn't specify a minimum ruby version, but if you try to use the gem with < 2.1, it breaks because of the required keyword arguments that were introduced in 2.1.

The changes in this PR do "backport" this behavior to 2.0. I couldn't find anything else other than these syntax changes that would make the gem incompatible with 2.0, and tests seem to pass.

Thoughts?


This change is Reviewable

@bbonamin
Copy link
Contributor Author

bbonamin commented Dec 20, 2016

Something seems to be up with TravisCI trying to install fsevents, which is not compatible with Linux. The build was green when I removed it from the npm shrinkwrap file on my fork, but of course that was a change that I didn't want to include in this pull request.

EDIT: Seems to be related to this issue npm/npm#14042, and it's apparently fixable by downgrading npm or running shinkrwrap again with npm 3.10.10+

@coveralls
Copy link

Coverage Status

Coverage remained the same at 82.75% when pulling 52e2915 on bbonamin:ruby20 into 93fb10b on shakacode:master.

@bbonamin bbonamin mentioned this pull request Dec 20, 2016
@coveralls
Copy link

coveralls commented Dec 20, 2016

Coverage Status

Coverage remained the same at 82.75% when pulling 137d186 on bbonamin:ruby20 into 93fb10b on shakacode:master.

@coveralls
Copy link

coveralls commented Dec 20, 2016

Coverage Status

Coverage remained the same at 82.75% when pulling a458267 on bbonamin:ruby20 into 93fb10b on shakacode:master.

@justin808
Copy link
Member

@samnang @mapreal19 @robwise This one looks OK to me. Ugly, but I guess some people need it.

I vote for this one. How about you guys?

@samnang
Copy link
Contributor

samnang commented Dec 22, 2016

@justin808 To me, it's okay if we want to support them for a while and pay attention of features that might break for this, but it shouldn't a long term because Rails 5 already drop their support. If we just trying with minimum dependency on Rails, then it might not be a problem.

@mapreal19
Copy link
Contributor

@justin808 @bbonamin we could remove the duplication for those required arguments with a helper method. Take a look at this answer http://stackoverflow.com/a/15078852/2273578

I would rather see those methods like this:

def  rails_context(server_side: required) 

Review status: 0 of 7 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@robwise
Copy link
Contributor

robwise commented Dec 23, 2016

lol I vote for this just the way I voted for it the last time someone did this almost a year ago, this repo really hardly uses keyword arguments so it's a no-brainer to keep it backwards compatible. I'm not sure why we keep re-introducing breaking changes:

https://github.com/shakacode/react_on_rails/blob/28776cbc36b22dc3aa91a5b1fe9f9b2b3c20fdc7/CHANGELOG.md#210---2016-01-26

@bbonamin bbonamin force-pushed the ruby20 branch 2 times, most recently from 12053d9 to 7edc2d0 Compare December 23, 2016 21:33
@bbonamin
Copy link
Contributor Author

@mapreal19 I've pushed a change with a DRYer approach to doing to required validation. Let me know if you think it's good 😃

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 83.198% when pulling 7edc2d0 on bbonamin:ruby20 into 501d593 on shakacode:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 83.198% when pulling 7edc2d0 on bbonamin:ruby20 into 501d593 on shakacode:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 83.198% when pulling 7edc2d0 on bbonamin:ruby20 into 501d593 on shakacode:master.

@coveralls
Copy link

coveralls commented Dec 23, 2016

Coverage Status

Coverage increased (+0.02%) to 82.775% when pulling 7edc2d0 on bbonamin:ruby20 into 501d593 on shakacode:master.

@justin808
Copy link
Member

Please rebase your changes on top of my latest commit.

@bbonamin
Copy link
Contributor Author

Done 👍 ! Merry Christmas! 🎄

@coveralls
Copy link

coveralls commented Dec 25, 2016

Coverage Status

Coverage increased (+0.006%) to 99.292% when pulling ab689f0 on bbonamin:ruby20 into 8bcaa34 on shakacode:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.006%) to 99.292% when pulling ab689f0 on bbonamin:ruby20 into 8bcaa34 on shakacode:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.006%) to 99.292% when pulling ab689f0 on bbonamin:ruby20 into 8bcaa34 on shakacode:master.

@justin808
Copy link
Member

Small suggestions @bbonamin.

Looking good!


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


app/helpers/react_on_rails_helper.rb, line 263 at r3 (raw file):

  def server_rendered_react_component_html(
    props, react_component_name, dom_id, prerender: required("prerender"),
    trace: required("trace"), raise_on_prerender_error: required("raise_on_prerender_error")

We should do these one per param per line. Easier to read.


lib/react_on_rails/test_helper/webpack_assets_status_checker.rb, line 14 at r3 (raw file):

      def initialize(
        generated_assets_dir: raise(ArgumentError, "generated_assets_dir is required"),
        client_dir: raise(ArgumentError, "client_dir is required"),

can we use the helper required here?


Comments from Reviewable

@justin808
Copy link
Member

Review status: all files reviewed at latest revision, 3 unresolved discussions.


CHANGELOG.md, line 9 at r3 (raw file):

*Please add entries here for your pull requests.*
##### Fixed
- Added support for Ruby 2.0.

Please see the other changelog entries and make consistent.


Comments from Reviewable

@justin808
Copy link
Member

Please put your changelog comments under 6.3.3.

It's not compatible with Ruby < 2.2
Required keyword arguments were introduced in Ruby 2.1, but can be emulated in 2.0 by using a default value that raises an ArgumentError

Add notes to CHANGELOG
To work around the issue npm/npm#14042,
where npm tries to install optional dependencies and fails.
@bbonamin bbonamin force-pushed the ruby20 branch 3 times, most recently from 646e9bc to b30045b Compare December 26, 2016 00:14
@bbonamin
Copy link
Contributor Author

@justin808 Thanks for the feedback! I've addressed your comments, and added a required_ruby_version to the gemspec (http://guides.rubygems.org/specification-reference/), so it's picked up by rubygems.org

So that rubygems.org can pick it up and display it on the gem page.
@justin808
Copy link
Member

:lgtm:

GREAT JOB!


Reviewed 5 of 5 files at r4.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


Comments from Reviewable

@justin808 justin808 merged commit eb54c08 into shakacode:master Dec 26, 2016
@coveralls
Copy link

Coverage Status

Coverage increased (+0.008%) to 99.294% when pulling 14488d2 on bbonamin:ruby20 into ccd970a on shakacode:master.

1 similar comment
@coveralls
Copy link

coveralls commented Dec 26, 2016

Coverage Status

Coverage increased (+0.008%) to 99.294% when pulling 14488d2 on bbonamin:ruby20 into ccd970a on shakacode:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.008%) to 99.294% when pulling 14488d2 on bbonamin:ruby20 into ccd970a on shakacode:master.

@bbonamin bbonamin deleted the ruby20 branch December 26, 2016 01:56
@mapreal19
Copy link
Contributor

:lgtm: Thanks @bbonamin!


Reviewed 1 of 7 files at r1, 2 of 3 files at r2, 1 of 1 files at r3, 5 of 5 files at r4.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks broke.


Comments from Reviewable

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.

6 participants