Skip to content

Commit

Permalink
VersionChecker only warns if major versions are different
Browse files Browse the repository at this point in the history
  • Loading branch information
robwise committed Jan 14, 2016
1 parent 5938d52 commit 976c89e
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 20 deletions.
13 changes: 11 additions & 2 deletions lib/react_on_rails/version_checker.rb
Expand Up @@ -16,14 +16,14 @@ def initialize(node_package_version, logger)
# For compatibility, the gem and the node package versions should always match, unless the user
# really knows what they're doing. So we will give a warning if they do not.
def warn_if_gem_and_node_package_versions_differ
return if node_package_version.normalized == gem_version || node_package_version.relative_path?
return if node_package_version.major == gem_major_version || node_package_version.relative_path?
log_differing_versions_warning
end

private

def log_differing_versions_warning
msg = "**WARNING** ReactOnRails: ReactOnRails gem and node package versions do not match\n" \
msg = "**WARNING** ReactOnRails: ReactOnRails gem and node package major versions do not match\n" \
" gem: #{gem_version}\n" \
" node package: #{node_package_version.raw}\n" \
"Ensure the installed version of the gem is the same as the version of your installed node package"

This comment has been minimized.

Copy link
@justin808

justin808 Jan 14, 2016

Member

installed MAJOR version

Expand All @@ -33,6 +33,10 @@ def log_differing_versions_warning
def gem_version
ReactOnRails::VERSION
end

def gem_major_version
gem_version.match(/(\d+)\..*/)[1]

This comment has been minimized.

Copy link
@justin808

justin808 Jan 14, 2016

Member

do you need the .*?

end
end

class NodePackageVersion
Expand Down Expand Up @@ -66,6 +70,11 @@ def relative_path?
normalized.nil? # must be a relative path if nil and we haven't failed
end

def major
return if normalized.nil?
normalized.match(/(\d+)\..*/)[1]

This comment has been minimized.

Copy link
@justin808

justin808 Jan 14, 2016

Member

same question as above

This comment has been minimized.

Copy link
@justin808

justin808 Jan 14, 2016

Member

generally you would need the .* if you're anchoring the regexp by \A \z to get the WHOLE string.

This comment has been minimized.

Copy link
@samnang

samnang Jan 14, 2016

Contributor

Yep, at that time as I understand from Rob is to capture the whole number instead of only get major release version. We don't need it for sure if we only need major number.

end

private

def package_json_contents
Expand Down
2 changes: 1 addition & 1 deletion spec/react_on_rails/fixtures/normal_package.json
@@ -1,7 +1,7 @@
{
"dependencies": {
"babel": "^6.3.26",
"react-on-rails": "^2.0.0.beta-2",
"react-on-rails": "^14.0.0.beta-2",
"webpack": "^1.12.8"
},
"devDependencies": {
Expand Down
46 changes: 29 additions & 17 deletions spec/react_on_rails/version_checker_spec.rb
Expand Up @@ -14,8 +14,8 @@ module ReactOnRails
describe "#warn_if_gem_and_node_package_versions_differ" do
let(:logger) { FakeLogger.new }

context "with equal gem and node packages" do
let(:node_package_version) { double_package_version(raw: "^2.0.0.beta-2", normalized: "2.0.0.beta.2") }
context "when gem and node package major versions are equal" do
let(:node_package_version) { double_package_version(raw: "^2.2.5", normalized: "2.2.5", major: "2") }

This comment has been minimized.

Copy link
@samnang

samnang Jan 14, 2016

Contributor

in the requirement if we only care about major, I'm not sure we may don't need normalized at all.

before { stub_gem_version("2.0.0.beta.2") }

it "does not log a warning" do
Expand All @@ -24,18 +24,22 @@ module ReactOnRails
end
end

context "with difffering gem and node packages" do
let(:node_package_version) { double_package_version(raw: "^2.0.0.beta-2", normalized: "2.0.0.beta.2") }
before { stub_gem_version("2.0.0.beta.1") }
context "when gem and node package major versions differ" do
let(:node_package_version) do
double_package_version(raw: "13.0.0.beta-2", normalized: "13.0.0.beta.2", major: "13")
end
before { stub_gem_version("12.0.0.beta.1") }

it "logs a warning" do
check_version(node_package_version, logger)
expect(logger.message).to be_present
end
end

context "with relative path to node package" do
let(:node_package_version) { double_package_version(raw: "../../..", normalized: "", relative_path: true) }
context "when package json uses a relative path" do
let(:node_package_version) do
double_package_version(raw: "../../..", normalized: "", major: "", relative_path: true)
end
before { stub_gem_version("2.0.0.beta.1") }

it "does not log a warning" do
Expand All @@ -49,8 +53,8 @@ def stub_gem_version(version)
stub_const("ReactOnRails::VERSION", version)
end

def double_package_version(raw:, normalized:, relative_path: false)
instance_double(NodePackageVersion, raw: raw, normalized: normalized, relative_path?: relative_path)
def double_package_version(raw:, normalized:, major:, relative_path: false)
instance_double(NodePackageVersion, raw: raw, normalized: normalized, major: major, relative_path?: relative_path)
end

def check_version(node_package_version, logger)
Expand All @@ -60,37 +64,45 @@ def check_version(node_package_version, logger)
end

describe NodePackageVersion do
subject(:node_package) { NodePackageVersion.new(package_json) }
subject(:node_package_version) { NodePackageVersion.new(package_json) }

This comment has been minimized.

Copy link
@justin808

justin808 Jan 14, 2016

Member

maybe a few tests like this? (adding a method on this class to give you the major_version)

subject(:node_package_major_version) { NodePackageVersion.new(package_json).major_version }

and why 2 classes in one file -- should be one class per file.

This comment has been minimized.

Copy link
@robwise

robwise Jan 14, 2016

Author Contributor

I think Samnang initially thought we should nest them, but we never got around to it, I will do it now. NodePackageVersion is sort of weird unless it's in the context of a version checker.

This comment has been minimized.

Copy link
@samnang

samnang Jan 14, 2016

Contributor

Usually have it nested class, but if you guys don't like them. Put them in another file.


context "when package json lists a version of '^2.0.0.beta-2'" do
context "when package json lists a version of '^14.0.0.beta-2'" do
let(:package_json) { File.expand_path("../fixtures/normal_package.json", __FILE__) }

describe "#raw" do
specify { expect(node_package.raw).to eq("^2.0.0.beta-2") }
specify { expect(node_package_version.raw).to eq("^14.0.0.beta-2") }
end

describe "#normalized" do
specify { expect(node_package.normalized).to eq("2.0.0.beta.2") }
specify { expect(node_package_version.normalized).to eq("14.0.0.beta.2") }

This comment has been minimized.

Copy link
@justin808

justin808 Jan 14, 2016

Member

throw in a test that does not have the beta stuff

This comment has been minimized.

Copy link
@samnang

samnang Jan 14, 2016

Contributor

The result shouldn't be different to have tests with beta version or not.

end

describe "#relative_path?" do
specify { expect(node_package.relative_path?).to be false }
specify { expect(node_package_version.relative_path?).to be false }
end

describe "#major" do
specify { expect(node_package_version.major).to eq("14") }
end
end

context "with node version of '../../..'" do
let(:package_json) { File.expand_path("../fixtures/relative_package.json", __FILE__) }

describe "#raw" do
specify { expect(node_package.raw).to eq("../../..") }
specify { expect(node_package_version.raw).to eq("../../..") }
end

describe "#normalized" do
specify { expect(node_package.normalized).to be_nil }
specify { expect(node_package_version.normalized).to be_nil }
end

describe "#relative_path?" do
specify { expect(node_package.relative_path?).to be true }
specify { expect(node_package_version.relative_path?).to be true }
end

describe "#major" do
specify { expect(node_package_version.major).to be_nil }
end
end
end
Expand Down

0 comments on commit 976c89e

Please sign in to comment.