Skip to content

Commit

Permalink
added lodash and react to dependencies list since peerDependencies wi…
Browse files Browse the repository at this point in the history 
…ll be deprecated from npm
  • Loading branch information
@mukeshsoni
mukeshsoni committed Jun 20, 2015
1 parent 038707f commit 860b56a
Showing 1 changed file with 2 additions and 0 deletions.
2 changes: 2 additions & 0 deletions package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@
"react": ">=0.12.1"
},
"dependencies": {
"lodash": ">=2.4.1",
"react": ">=0.12.1",
"classnames": "^2.1.2",
"react-onclickoutside": "^0.2.4"
}
Expand Down

7 comments on commit 860b56a

@KyleAMathews
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually this isn't right as this will make NPM potentially download additional versions of React leading to the dreaded bug facebook/react#1939 (I've seriously run into this maybe a dozen or more times)

@mukeshsoni
Copy link
Owner Author

Choose a reason for hiding this comment

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

But npm keeps spewing a warning that peerDependencies will be deprecated - webpack-contrib/url-loader#12.

As far as i know the only way to get around it is to ensure that library you use has a stricter version requirement for react and the consumer ensure at their end that the versions they have are similar to the version used in the third party library.

as for double downloads, i think bundlers like webpack and browserify might take care of it. In the latest setup the lib folder contains a bundled file for ReactTelephoneInput produced by browserify which does not have react bundled.

@KyleAMathews
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps the best thing to do is to not put React in dependencies or peerDependencies. I would assume anyone using this module already has React installed. Leaving Lodash as a dependency is less of a problem I think.

@mukeshsoni
Copy link
Owner Author

Choose a reason for hiding this comment

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

That is a fair assumption to make considering it's a react component :). And for testing, having react as devDependency should suffice. Only problem is we need to convey somehow the version of react this component is compatible with. For now it seems to be compatible with both 0.12.* and 0.13.*.

@mukeshsoni
Copy link
Owner Author

Choose a reason for hiding this comment

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

Some good discussion here too - formatjs/formatjs#79

I don't know if having the dependency range as "react": ">=0.11.2 <0.14.0" will solve the problem?

@iamstarkov
Copy link

Choose a reason for hiding this comment

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

Only problem is we need to convey somehow the version of react this component is compatible with.

Why not just state compatibility range in readme?

@mukeshsoni
Copy link
Owner Author

Choose a reason for hiding this comment

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

Is it not better to have an automated hard restriction which tells you about conflicts or non compatibility during the time of installation?

Please sign in to comment.