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

Update dom-helpers #522

Merged
merged 2 commits into from
Jan 3, 2017
Merged

Update dom-helpers #522

merged 2 commits into from
Jan 3, 2017

Conversation

danez
Copy link
Contributor

@danez danez commented Jan 2, 2017

Currently uses the older version, which leads to dom-helpers being included twice, when bundling react-virtualized with webpack and having dom-helpers 2.x and 3.x used.

@bvaughn
Copy link
Owner

bvaughn commented Jan 2, 2017

Hi @danez,

Thank you for creating this PR.

Jumping a dependency from 2.x to 3.x is a questionable change without a major release though, and I don't see a CHANGELOG for the dom-helpers lib unfortunately so I'm not sure if it would be safe enough to just relax the dependency to allow either 2.x or 3.x (eg "^2.4.0 || ^3.0.0")

@jquense would you have any suggestion by chance?

@jquense
Copy link
Contributor

jquense commented Jan 2, 2017

should be safe to upgrade. the "breaking" change in 2 -> 3 was around the import structure when importing all of dom-helpers, since RV just cherry-picks scrollbarSize , it shouldn't affect ya'll

@bvaughn
Copy link
Owner

bvaughn commented Jan 3, 2017

Excellent. Thanks @jquense!

@danez let's go with a pattern that supports either version (eg "^2.4.0 || ^3.0.0") and leave it up to library users which they use. I don't want to yank the rug out from under anyone who may be relying on the 2.x import structure in their applications. It sounds like RV is safe to work with either though. 😁

@danez
Copy link
Contributor Author

danez commented Jan 3, 2017

Okay updated :)

@bvaughn bvaughn merged commit a15078d into bvaughn:master Jan 3, 2017
@bvaughn
Copy link
Owner

bvaughn commented Jan 3, 2017

Fantastic. Thanks to both of your for your help with this. 😄

I'll try to get a release out sometime within the next day or two.

@danez danez deleted the patch-1 branch January 3, 2017 16:21
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

3 participants