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.

Sign up for GitHub

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

Jump to bottom

Version checker cannot cope with single digit strings in package.json #489

Closed
samsondav opened this issue Jul 26, 2016 · 7 comments
Closed

Version checker cannot cope with single digit strings in package.json #489

samsondav opened this issue Jul 26, 2016 · 7 comments
Labels
bug

Comments

@samsondav
Copy link
Contributor

Steps to reproduce:

  1. In client/package.json specify version: "react-on-rails": "^6"
  2. In root path run bundle exec rails server

Expected behaviour:

  • Should run fine

Actual behaviour:

  • Crashes with:
/Users/sam/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/react_on_rails-6.0.5/lib/react_on_rails/version_checker.rb:69:in `major': undefined method `[]' for nil:NilClass (NoMethodError)
    from /Users/sam/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/react_on_rails-6.0.5/lib/react_on_rails/version_checker.rb:21:in `warn_if_gem_and_node_package_versions_differ'
    from /Users/sam/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/react_on_rails-6.0.5/lib/react_on_rails/engine.rb:5:in `block in <class:Engine>'
    from /Users/sam/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/activesupport-5.0.0/lib/active_support/callbacks.rb:396:in `instance_exec'
    from /Users/sam/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/activesupport-5.0.0/lib/active_support/callbacks.rb:396:in `block in make_lambda'
(... trace truncated ...)

The problem is on version_checker.rb line 69. The regex should probably be more like this: raw.match(/(\d+)(\.|\z)/)[1]
@justin808
Copy link
Member

Hi @samphilipd!

Any chance that you can help us with a PR (including a test?)

CC: @robwise

@justin808 justin808 added the bug label Jul 27, 2016
samsondav pushed a commit to samsondav/react_on_rails that referenced this issue Jul 27, 2016
Add support for single digit version strings, closes shakacode#489
496b2b3
samsondav pushed a commit to samsondav/react_on_rails that referenced this issue Jul 28, 2016
Add support for single digit version strings, closes shakacode#489
48ddc00
samsondav pushed a commit to samsondav/react_on_rails that referenced this issue Jul 31, 2016
Add support for single digit version strings, closes shakacode#489
6279887
justin808 added a commit that referenced this issue Aug 1, 2016
@justin808
Merge pull request #491 from samphilipd/master
63436ab 
Add support for single digit version strings, closes #489
eacaps added a commit to eacaps/react_on_rails that referenced this issue Aug 1, 2016
@eacaps
allow component rendering in contexts without requests
219c6dc 
allow custom_context even without request

added broken test

added fixes to allow actionmailer test to pass

cleaned up rubocop warnings

Update ensure_assets_compiled.rb

resolved some reviewed suggestions

fix(typo) remove duplicated word in readme

updated with proper handles for inMailer

added inMailer to test

added inMailer checks for test

cleaned up rubocop suggestions

a few final minor tweaks

Fixes typo

Update server-rendering-tips.md (shakacode#494)

Update server-rendering-tips.md and README.md

Update README.md

Doc Fixes (shakacode#499)

* Update node-server-rendering.md
* Update README.md

Enhancements to webpack asset preparation

* Better messages when creating symlinks
* Updated documentation
* Enhanced example
* Support subdirectories with webpack assets
* Move logic for assets code to service object
* Using defaults of the env settings or else values for directories and
  regexp can be provided.

Update assets_precompile_spec.rb

symlink tests with tempfs

Move CONTRIBUTING.MD to project top level

This seems to make it show more prominently when making new issues or
PRs.

Remove Docker from setup

* Update .travis.yml
* Remove Dockerfile_tests and docker-compose.yml

fixed spelling error in readme

Update README.md

Add support for single digit version strings, closes shakacode#489
eacaps added a commit to eacaps/react_on_rails that referenced this issue Aug 2, 2016
@eacaps
Squashed commit of the following:
ec97365 
commit 7d4e7fe
Merge: cb9fab4 3c0e6d9
Author: eacaps <eacaps@gmail.com>
Date:   Mon Aug 1 11:46:35 2016 -0400

    Merge branch 'feature/no-request-context' of github.com:eacaps/react_on_rails into feature/no-request-context

commit cb9fab4
Author: eacaps <eacaps@gmail.com>
Date:   Thu Jul 21 13:23:51 2016 -0400

    allow component rendering in contexts without requests

    allow custom_context even without request

    added broken test

    added fixes to allow actionmailer test to pass

    cleaned up rubocop warnings

    resolved some reviewed suggestions

    updated with proper handles for inMailer

    added inMailer to test

    added inMailer checks for test

    cleaned up rubocop suggestions

    a few final minor tweaks

commit 3c0e6d9
Merge: 5637162 63436ab
Author: eacaps <eacaps@gmail.com>
Date:   Mon Aug 1 10:44:59 2016 -0400

    Merge branch 'master' into feature/no-request-context

commit 5637162
Author: eacaps <eacaps@gmail.com>
Date:   Mon Aug 1 10:35:40 2016 -0400

    a few final minor tweaks

commit 63436ab
Merge: b4cdfd2 d694df8
Author: Justin Gordon <justin.gordon@gmail.com>
Date:   Sun Jul 31 23:01:59 2016 -1000

    Merge pull request shakacode#491 from samphilipd/master

    Add support for single digit version strings, closes shakacode#489

commit d694df8
Author: Sam Davies <seivadmas@gmail.com>
Date:   Wed Jul 27 10:52:49 2016 +0100

    Add support for single digit version strings, closes shakacode#489

commit b4cdfd2
Author: Justin Gordon <justin.gordon@gmail.com>
Date:   Sun Jul 31 21:25:57 2016 -1000

    Update README.md

commit 8824b88
Merge: e449d84 442dcd4
Author: Justin Gordon <justin.gordon@gmail.com>
Date:   Sun Jul 31 21:25:35 2016 -1000

    Merge pull request shakacode#503 from markpenovich/master

    fixed spelling error

commit 442dcd4
Author: Mark Penovich <mpenovich1@gmail.com>
Date:   Sun Jul 31 23:47:20 2016 -0500

    fixed spelling error in readme

commit e449d84
Merge: 95efecd 8cec9cf
Author: Justin Gordon <justin.gordon@gmail.com>
Date:   Sun Jul 31 15:49:11 2016 -1000

    Merge pull request shakacode#497 from shakacode/justin808-skip-docker

    Remove docker from CI tests .travis.yml

commit 95efecd
Merge: 46ecf59 ef08742
Author: Justin Gordon <justin.gordon@gmail.com>
Date:   Sun Jul 31 15:48:51 2016 -1000

    Merge pull request shakacode#502 from shakacode/move-contributing-to-top-level

    Move CONTRIBUTING.MD to project top level

commit ef08742
Author: Justin Gordon <justin.gordon@gmail.com>
Date:   Sun Jul 31 15:06:11 2016 -1000

    Move CONTRIBUTING.MD to project top level

    This seems to make it show more prominently when making new issues or
    PRs.

commit 46ecf59
Merge: cdb246b a6e35fe
Author: Justin Gordon <justin.gordon@gmail.com>
Date:   Sun Jul 31 15:42:20 2016 -1000

    Merge pull request shakacode#479 from shakacode/alleycat-at-git-alexey/replace_symlinks_copy

    * Enhancements to webpack asset preparation
    * Better messages when creating symlinks
    * Updated documentation
    * Enhanced example
    * Support subdirectories with webpack assets
    * Move logic for assets code to service object
    * Using defaults of the env settings or else values for directories and
      regexp can be provided.

commit 8cec9cf
Author: Justin Gordon <justin.gordon@gmail.com>
Date:   Fri Jul 29 15:34:10 2016 -1000

    Remove Docker from setup

    * Update .travis.yml
    * Remove Dockerfile_tests and docker-compose.yml

commit a6e35fe
Author: dzirtusss <dzirtusss@gmail.com>
Date:   Tue Jul 26 18:43:20 2016 +0300

    Update assets_precompile_spec.rb

    symlink tests with tempfs

commit 07a6e48
Author: Justin Gordon <justin.gordon@gmail.com>
Date:   Sun Jul 17 23:23:03 2016 -1000

    Enhancements to webpack asset preparation

    * Better messages when creating symlinks
    * Updated documentation
    * Enhanced example
    * Support subdirectories with webpack assets
    * Move logic for assets code to service object
    * Using defaults of the env settings or else values for directories and
      regexp can be provided.

commit cdb246b
Author: Justin Gordon <justin.gordon@gmail.com>
Date:   Sat Jul 30 12:30:33 2016 -1000

    Doc Fixes (shakacode#499)

    * Update node-server-rendering.md
    * Update README.md

commit 1e4c0ed
Author: Justin Gordon <justin.gordon@gmail.com>
Date:   Sat Jul 30 11:16:25 2016 -1000

    Update README.md

commit 13f2cf0
Author: Justin Gordon <justin.gordon@gmail.com>
Date:   Thu Jul 28 13:38:29 2016 -1000

    Update server-rendering-tips.md (shakacode#494)

    Update server-rendering-tips.md and README.md

commit 0604006
Merge: 8ca86ed e69480a
Author: Justin Gordon <justin.gordon@gmail.com>
Date:   Wed Jul 27 22:14:17 2016 -1000

    Merge pull request shakacode#492 from cubadomingo/patch-1

    Fixes typo

commit e69480a
Author: Devin Osorio <me@devinosor.io>
Date:   Wed Jul 27 22:34:13 2016 -0400

    Fixes typo

commit 566d62f
Author: eacaps <eacaps@gmail.com>
Date:   Mon Jul 25 12:59:44 2016 -0400

    cleaned up rubocop suggestions

commit e24fb9f
Author: eacaps <eacaps@gmail.com>
Date:   Mon Jul 25 11:28:56 2016 -0400

    added inMailer checks for test

commit eb6e700
Author: eacaps <eacaps@gmail.com>
Date:   Mon Jul 25 11:14:41 2016 -0400

    added inMailer to test

commit 8d34353
Author: eacaps <eacaps@gmail.com>
Date:   Mon Jul 25 10:41:35 2016 -0400

    updated with proper handles for inMailer

commit 52f0cbb
Merge: 840bc62 8ca86ed
Author: eacaps <eacaps@gmail.com>
Date:   Mon Jul 25 10:27:23 2016 -0400

    Merge branch 'master' into feature/no-request-context

commit 8ca86ed
Merge: 9a8b54f 0257cae
Author: Justin Gordon <justin.gordon@gmail.com>
Date:   Fri Jul 22 21:35:57 2016 -1000

    Merge pull request shakacode#487 from jooohn/fix/typo-in-readme

    fix(typo) remove duplicated word in readme

commit 0257cae
Author: jooohn <jooohn12341234@gmail.com>
Date:   Sat Jul 23 13:12:37 2016 +0900

    fix(typo) remove duplicated word in readme

commit 840bc62
Author: eacaps <eacaps@gmail.com>
Date:   Fri Jul 22 09:57:28 2016 -0400

    resolved some reviewed suggestions

commit 9b958fb
Merge: 6fdb73a 9a8b54f
Author: eacaps <eacaps@gmail.com>
Date:   Fri Jul 22 09:56:35 2016 -0400

    Merge branch 'master' into feature/no-request-context

commit 9a8b54f
Merge: e6afa98 4890486
Author: Justin Gordon <justin.gordon@gmail.com>
Date:   Thu Jul 21 17:42:32 2016 -1000

    Merge pull request shakacode#483 from shakacode/justin808-inaccurate-build-test-message

    Update ensure_assets_compiled.rb

commit 6fdb73a
Author: eacaps <eacaps@gmail.com>
Date:   Thu Jul 21 16:35:42 2016 -0400

    cleaned up rubocop warnings

commit d99fc36
Author: eacaps <eacaps@gmail.com>
Date:   Thu Jul 21 14:46:20 2016 -0400

    added fixes to allow actionmailer test to pass

commit 05d3144
Author: eacaps <eacaps@gmail.com>
Date:   Thu Jul 21 14:31:19 2016 -0400

    added broken test

commit 610906e
Author: eacaps <eacaps@gmail.com>
Date:   Thu Jul 21 13:25:42 2016 -0400

    allow custom_context even without request

commit 1e9d58c
Author: eacaps <eacaps@gmail.com>
Date:   Thu Jul 21 13:23:51 2016 -0400

    allow component rendering in contexts without requests

commit 4890486
Author: Justin Gordon <justin.gordon@gmail.com>
Date:   Wed Jul 20 19:55:39 2016 -1000

    Update ensure_assets_compiled.rb
@RKushnir
Copy link
Contributor

RKushnir commented Aug 4, 2017

This issue is still present in 8.0.6.
I installed the js package as follows: yarn add react-on-rails@8, and the gem tries to parse "8" with "major.minor.patch" regex.

@RKushnir
Copy link
Contributor

RKushnir commented Aug 4, 2017

So it's been reverted to the broken version here 251fa75#diff-0c19a2d62c614c09b80b965b9aa73eac

This issue needs to be re-opened.

@justin808
Copy link
Member

@RKushnir We require the full X.Y.Z version number to stay in sync between the gemfile and the package.json. The reason:

In case we deploy a change that has a bug, we may need to update both the Gem and the NPM version and require both are at the same exact version.

@RKushnir
Copy link
Contributor

RKushnir commented Aug 4, 2017

@justin808 I understand that, but the parser should tell what the problem is, not just fail with a random exception. To figure it out, I had to explore the gem source and debug the reason of failure, quite inefficient.

@justin808
Copy link
Member

@RKushnir Any chance that you can submit a PR for a better error message?

@justin808 justin808 reopened this Aug 4, 2017
@justin808
Copy link
Member

Closing for lack of interest.

@squadette squadette mentioned this issue Feb 7, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@RKushnir @justin808 @samsondav