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

is it compatible with React 0.15? #10

Open
plandem opened this issue Jan 31, 2017 · 11 comments
Open

is it compatible with React 0.15? #10

plandem opened this issue Jan 31, 2017 · 11 comments

Comments

@plandem
Copy link

plandem commented Jan 31, 2017

I have warnings about manually validating PropType:

Warning: You are manually calling a React.PropTypes validation function for the `routing` prop on `Controller`. This is deprecated and will not work in production with the next major version. You may be seeing this warning due to a third-party PropTypes library. See https://fb.me/react-warning-dont-call-proptypes for details.
@erikdstock
Copy link

@plandem - can you give a little more context on where this is happening?

@plandem
Copy link
Author

plandem commented Feb 1, 2017

@erikdstock probably it does not matter now, because:
a) we don't use ramda, so I had to fork and replace ramda with lodash
b) we needed a way to handle errors from generators and there is opened issue(#8) about this for ages, so I had to fix it.
c) after all these changes - I can't reproduce this warning anymore.

P.S.: if you want, you can check commits here:
https://github.com/plandem/react-redux-controller

@acjay
Copy link
Contributor

acjay commented Feb 1, 2017

We're using with React 0.15 at Artsy with no warnings. react-redux-controller doesn't call any PropTypes validations, it just shuttles them between objects. I'm guessing it's happening elsewhere. My guess was maybe react-redux, but they've got a similar issue where they say they're not at fault (reduxjs/react-redux#484). But clearly you've gone through all the code, so feel free to correct me if you see that call happening someplace.

Glad you're finding value with the project, and cool to see you've extended it.

@plandem
Copy link
Author

plandem commented Feb 1, 2017

@acjay I would like to use your npm package directly, but have no idea how to replace ramda with lodash without any headache. The only way - replace these 3 functions(map/flatten/pick) with own implementation :(

@acjay
Copy link
Contributor

acjay commented Feb 1, 2017

I'm all for Lodash, although I'm partial to the lodash-fp API flavor, since it preserves the immutability and data-last aspects of Ramda. (I don't really think we really put data-last to work in this project, but I was convinced of the merits by things such as the links under Introductions on the Ramda home page.)

@plandem
Copy link
Author

plandem commented Feb 2, 2017

  1. lodash/fp is based on lodash
  2. I already replaced ramda with lodash and as result only 3 functions from lodash are using: mapValues, flattenDeep, pick. Actually, I don't see any reason for lodash-fp here - result already immutable in case of these functions.

But why do you need data-last aspect?

@acjay
Copy link
Contributor

acjay commented Feb 2, 2017

Yeah, same project, just a different "flavor", interface, or however you want to call it :)

Yep, looking at how you did the the conversion to Lodash, I agree that you're effectively working immutably. But one thing I appreciate about the lodash/fp is that everything is immutable to begin with, whereas with the original Lodash, it varies across the API, so there's no need to reason about it at all. I've gotten bitten in the past with subtle mutations of references passed into a helper method.

As I said earlier, we're not currently leveraging data-last to any real advantage. So I agree that it's a toss-up here. It's just my sense that it's the better default stylistic choice, because it scales better to more interesting usages. If you haven't checked out the links Ramda references, you might find some of them compelling!

@plandem
Copy link
Author

plandem commented Feb 3, 2017

I can create a separate branch with this changes to help you :)

@acjay
Copy link
Contributor

acjay commented Feb 4, 2017

That sounds great, thanks!

I'm hoping to have some time to put some maintenance into this project soon :) It's minimal enough that it doesn't need that much love, but I'd still like to make a few tweaks.

@plandem
Copy link
Author

plandem commented Feb 6, 2017

will try to commit today

P.S.:
I really like your concept of controller/sync(for async) actions more than redux-saga or based on it dva.

before I used vanilla redux and was trying to find way to improve/decouple some part of application. Your solution is tiny yet powerful....with vanilla redux same time.

@plandem
Copy link
Author

plandem commented Feb 10, 2017

@acjay, I created pull request for branch with lodash, #13

p.s.: only one helper function disaggregateSuperSelector is not refactored - I never used it and there is no usage at library, so I omit 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

No branches or pull requests

3 participants