search

artsy / react-redux-controller

Library for creating a controller layer to link React and Redux, on top of react-redux.
MIT License
97 stars 10 forks source link

refactor for lodash usage + deinitialize #13

Closed plandem closed 6 years ago

plandem commented 7 years ago

*refactor for lodash usage +added deinitialize() logic to componentWillUnmount

acjay commented 7 years ago

Sorry for the delay -- I'm going to give this a test just to make sure it works with application that depends on it, and if so, I'll go ahead and merge!

plandem commented 7 years ago

don't forget about disaggregateSuperSelector, library is not using it, so I commented it.

acjay commented 7 years ago

I'm gonna try try try to test this with our app in the next day or two

plandem commented 7 years ago

still no any news?! :)

damassi commented 7 years ago

@plandem - Why the decision to switch to lodash and move away from ramda? If its to reduce the bundle size via plugin there is https://www.npmjs.com/package/babel-plugin-ramda which is essentially the same thing. IMO I find Ramda to be far more readable due to the data-last quality of it, and also more flexible due to currying.

plandem commented 7 years ago

e.g. redux uses lodash, so less differ dependencies for lib.

damassi commented 7 years ago

I'll let @acjay chime in here, but I could be open to seeing this refactored using the new import { select } from 'lodash/fp' style now baked into the core lib, but I'm somewhat hesitant to switch the arguments around so that data comes first because then it looses some of its flexibility.

artsy-peril[bot] commented 6 years ago
Warnings
:warning: Please assign someone to merge this PR, and optionally include people who should review.

Generated by :no_entry_sign: dangerJS