search

brianegan / bansa

A state container for Java & Kotlin, inspired by Redux & Elm
MIT License
444 stars 29 forks source link

first version for reselect. state selector library ported from https:… #6

Closed beyondeye closed 8 years ago

beyondeye commented 8 years ago

Hi. After playing a bit with your library and looking into the original docs for Redux I realized that there was quite an important missing piece. Checking in all subscribers to see if the state change that happened is relevant or not can be much easier with reselect library. see also http://redux.js.org/docs/recipes/ComputingDerivedData.htm) and https://github.com/reactjs/reselect This is not a complete port of reselect, but I hope good enough as for a start. Also I strived to make the API as simple as possible and changed it to be more idiomatic for kotlin. Let me know what you think. Thanks for your work on bansa, by the way!

brianegan commented 8 years ago

Oh dang! Super cooooool!!! I'll take a deeper look at this in the next day or so! Thanks so much for the contribution :)

beyondeye commented 8 years ago

Hi. I have improved quite a bit from the initial commit. Did you have time to look at it? Also, when we are done with this PR, I will open a PR for a Promise middleware I have written, and some other improvements: I don't like too much the fact that applyMiddleware() take as input the curried middleware function. It would be easier for middleware writers to write the uncurried version and make bansa take care of the currying, like this: `private fun<P1, P2, P3, R> Function3<P1, P2, P3, R>.curried(): (P1) -> (P2) -> (P3) -> R { return { p1: P1 -> { p2: P2 -> { p3: P3 -> this(p1, p2, p3) } } } }

fun <S, A> Store<S,A>.applyMiddleware( vararg middleware: (Store<S, A>,(A)->A ,A ) -> A ): Store<S, A> { dispatch = compose(middleware.map { it.curried()(this) })(dispatch) return this }`

brianegan commented 8 years ago

Hey, thanks again! Sorry, super busy week from my end. I'll take a look this evening / tomorrow for sure!

brianegan commented 8 years ago

With regards to your second comment about currying the function for the user, I completely agree. I went this route initially to stay more closely inline with the original Redux implementation, but it's really annoying and noisy to write middleware currying all the time. In fact, I even created a helper function in one of the examples to help out with that exact problem, haha:

https://github.com/brianegan/bansa/blob/master/examples/listOfTrendingGifs/src/main/kotlin/com/brianegan/bansa/listOfTrendingGifs/middleware/createMiddleware.kt

So yeah, I'll take a look at this stuff, and I'd be happy to see a PR changing the way applyMiddleware works. Thanks again for all your thoughts and contributions!

brianegan commented 8 years ago

Thanks so much for your patience! I really wanted to look through this code thoroughly and give you good feedback :)

I'll leave a few more comments inline, but here are a few general thoughts:

Thanks again for the contribution. These are all open questions, and I'm totally open to discussions on these points.

beyondeye commented 8 years ago

Hi, thank you a lot for the code review and suggestion! I will try fix the style issues tomorrow and address your questions. About having the reselect library in a separate repository: it makes a lot of sense, from the point of view of dependencies ( the absence of) from bansa. On the other hand I think that if you are interested in more people using and contributing to bansa, it would help to have several useful modules in the same code repository. In reduxjs is quite annoying to go and search for each module in a separate repository. At least this is what I think.

beyondeye commented 8 years ago

Hey, done with the fixes! some answers to your questions:

protected var _recomputations:

the _ at the beginning of _recomputations is actually the style for naming backing properties used in the Kotlin reference (see https://github.com/JetBrains/kotlin-web-site/blob/master/docs/reference/properties.md#backing-properties)

renaming class SelectorFor:

I understand your comment, but what's the difference between AssertThat and SelectFor? both are builders by the way.

Personal opinion: Something about creating selectors doesn't quite work for me. Perhaps. Does it need to be all class-based with builders, or could it have a more functional interface? Instead of withField, couldn't that be a simple function { state -> state.a }? I guess what I'm saying: Does SelectorInput need to be an interface and Selector inherit from that? Seems it could be simple functions wrapped in memoize calls. What do you think?

I tried to do that, but kotlin type inference in the compiler does not work if I pass directly lambdas to the builder. Don't ask me why. I have tried again, adding at least a shortcut that allow to pass directly a function for the case of single selector. It crashes the compiler when I try to run the the test singleFieldSelectorShortSyntaxText

Do you think there is merit in Selectors being able to accept arguments? Instead of only taking the state, you could have a selector like: getUser(userUrn: Urn, state: AppState), for example?

Yes I want to implement this feature, but did not yet decide the best way to do it

Do you think it's a good idea to manage recomputations and allow for resetting computations? I know that's in the original library and is included to aid unit testing, but I feel like that's leaking testing code into the core path of the library. What are your thoughts? Would downstream authors care about how often something is recomputed? If needed, it could be a helper function that wraps this stuff, and only used for unit tests. Feels like the contract with downstream authors should "memoization just works."

ResetComputations removed

brianegan commented 8 years ago

Thanks for taking the time to update this a bit more! Cool to see the progress :)

After thinking about it a bit, I have to say, I'm not sure I want to bring this into the core Bansa library. I really appreciate what you've created and totally encourage you to package it up into a separate library! Heck, I'd even help with that!

I'm actually going to focus on making Bansa smaller in footprint in the near future. While caching and memoization are helpful, I'd like to keep the core clean and small so folks can quickly grasp the basics. Then, we can create add-on libs that provide additional functionality, but don't have to move away from the core of what Bansa is: A simple state container.

That said, happy to continue with the review and discussion!

"I tried to do that, but kotlin type inference in the compiler does not work if I pass directly lambdas to the builder."

Ah, bummer! That'd be really cool to get this working with a more functional interface. But such is the nature of the language!

"I understand your comment, but what's the difference between AssertThat and SelectFor? both are builders by the way."

Yah I totally get what you're saying. In fact, I think there's a win-win here, just some small tweaks.

In my eye, SelectorFor just looks funny. The simple reason: SelectorFor is a class, and classes should be named using good ol' nouns. I'd say it differs importantly from assertThat, because assertThat is a method (verb), whereas SelectFor is a class (should be a simple noun). That's why I'd recommend the following name changes:

SelectorFor -> SelectorBuilder or Selector.Builder just to keep it super clear. SelectorForPN -> SelectorBuilderPN (e.g. SelectorForP1 -> SelectorBuilderP1)

But hey, it's your library so do with it as you please :)

Once again, thanks so much for the contribution! I'd like to keep Bansa small, and am willing to help publish this in a separate repo. But I'd like to keep the two separate for now as I figure out how to make Bansa itself leaner.