Khan / react-components

khan.github.io/react-components/
MIT License
1.01k stars 99 forks source link

Sortable no longer depends on Underscore #43

Closed sloria closed 6 years ago

sloria commented 9 years ago
kevinbarabash commented 9 years ago

@sloria I think in general it makes sense to remove dependencies so that react-components is more lightweight. I haven't actually used this library, is it possible to require this component on its own?

sophiebits commented 9 years ago

Another thing we could consider is depending on lodash.difference which only pulls in that one function. +1 for builtin map and forEach though.

sloria commented 9 years ago

@spicyj lodash.difference might still be more LOC than it's worth. It includes code to handle different input types and variable arity--the sortable component has just two usages of difference, for which both the input types and arity are always the same.

Another downside is that if you are already using the full lodash package in your project, depending on lodash.difference will add duplicated code to your vendor bundle.

sophiebits commented 9 years ago

Your approach adds duplicated code to your bundle regardless of whether you're using lodash. There's not a practical difference between 10 and 50 lines here and I'm much happier to depend on a well-maintained library like lodash rather than reimplementing things and having the potential for bugs.

sophiebits commented 9 years ago

If we use Array.prototype.filter I guess the one-line difference function here is fine.

sloria commented 9 years ago

@spicyj I originally used Array.prototype.filter, but this breaks because we need to filter a NodeList here, which doesn't implement filter.

sophiebits commented 9 years ago

Did you see my other comment? Array.prototype.filter.call(nodeList, pred) should work fine.

sloria commented 9 years ago

@spicyj Ah, sorry, yeah, just caught your other comment. Yes, that will work; I'll change the code.

kevinbarabash commented 6 years ago

@sloria these changes have been made by another commit. Sorry for not merging this sooner.

sloria commented 6 years ago

@kevinbarabash No problem. Glad to see the change go in.