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

Depend on updated code-dot-org/react-color #8274

Merged
merged 1 commit into from May 9, 2016

Conversation

islemaster
Copy link
Contributor

Upstream https://github.com/casesandberg/react-color has been updated to 2.1.0 with support for React 15+. Unfortunately it still depends on the latest published react-css, which still depends on React 14 (though that repository https://github.com/casesandberg/reactcss has received the requisite update). I'm concerned that depending directly on the npm package we may end up including two versions of React again.

I updated our react-css and react-color forks to match their upstreams, except that they keep the problematic dependency chain within our own org. This change points at the updated code-dot-org/react-color, which in turn is pointing to the updated code-dot-org/reactcss.

As soon as reactcss gets a version bump published to npm we should be able to depend directly on react-color@2.1.0 again.

Our react-color fork has been updated to match casesandberg/react-color, except that it still depends on our own reactcss fork.  I'd just set our dependency back to npm react-color@2.1.0 but it's still pointing to npm's reactcss which didn't get a version bump for its react 15 fix and probably still depends on react 14.  Keeping our fork for now to ensure we don't ship more than one version of react.
@Hamms
Copy link
Contributor

Hamms commented May 9, 2016

What's the motivation for reactcss to depend on react v14 || v15 rather than just 15?

@islemaster
Copy link
Contributor Author

I thought since the package still functions perfectly well with React 0.14, it's reasonable to define a looser dependency in the hope that it will be more forgiving when used with other packages that only support React 0.14 and be able to find one version that satisfies all of them. Of course, in those cases one could simply depend on an older version of react-color, but if the newer one genuinely works, why lock it down?

I suppose the counterargument is that reactcss and react-color don't regularly test against React 0.14, since their own CI is going to prefer the latest version it can get. That strikes me as a problem with any loose dependency though, including something like ^15.0.0.

Thoughts?

@Hamms
Copy link
Contributor

Hamms commented May 9, 2016

I suppose it comes down to which we value more, flexibility or security. Personally, I'd lean toward the simplicity of relying on only one major version, but I can definitely see it going either way.

And yeah, that can be a problem with NPM's version specification scheme in general; it's less of an issue for something big and relatively reliable like React, but losely depending on more obscure packages carries with it the risk of a minor version update including some breaking change. You can't always trust developers to use semantic versioning properly :P

@Hamms
Copy link
Contributor

Hamms commented May 9, 2016

Either way, though, this LGTM

@islemaster
Copy link
Contributor Author

I generally like the idea of a module developer providing more flexibility (and hopefully testing against a few versions), and then expecting applications to use something like shrinkwrap to manage changes carefully.

I don't know what best practice really is here though, or if it's well-defined at all.

Either way, I suggested it, and the maintainer accepted the PR. If they change their mind and only support React 15, we're still good to go.

Thanks for discussing! I'm still trying to understand the best way to work within the npm ecosystem.

@islemaster islemaster merged commit f95215c into staging May 9, 2016
@islemaster islemaster deleted the update-react-color-dependency branch May 9, 2016 19:47
@Bjvanminnen
Copy link
Contributor

I think the best practice for this these days in the npm ecosystem is to define React 14 || 15 as a peerDependency, though I also haven't taken the time to do a lot of learning here.

@davidsbailey
Copy link
Member

If we are not in a hurry to upgrade react-color, then it seems simplest and safest to wait until react-color supports React 15 as a dependency before upgrading it.

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.

None yet

4 participants