chore(selectors): refactor selectors

[Dattaya]

• Simplified regular selectors
• Gave names recommended by the reselect docs to selector factories (proposed by @kachkaev, thanks!)

[coveralls]

Coverage remained the same at 98.605% when pulling 6772d30 on Dattaya:selector-factories into dd8f9a6 on mxstbr:dev.

[spen]

Awesome work :) I feel this gives a lot more clarity so should help people avoid falling in to anti-patterns :D

I do have one more thing to ask / suggest...

I'm not particularly sold on the naming: makeSelectLocationState.
I feel this is a little confusing since make & select are both verbs.

My suggestion would be something like:
create______Selector

so makeSelectLocationState becomes createLocationStateSelector

I feel that might either be confused with createSelector() or could actually be more inline with it... honestly it's all fairly new to me so I could be way off the mark.
Any thoughts?

Edit: I see that reselect give the example of makeGetVisibleTodos here
So it makes sense to stay true to that example - I still find the verb-verb thing a bit icky though! :P

[Dattaya]

@spen, makes sense, two consecutive verbs do sound weird, if more people are in favor of "createLocationStateSelector" I'll redo the naming in this PR.

[coveralls]

Coverage remained the same at 98.605% when pulling 2b40821 on Dattaya:selector-factories into ead6cb8 on mxstbr:dev.

[Dattaya]

Everyone here (@spen, @gihrig, and I) is in favor of create...Selector names for selector factories, in reduxjs/reselect#190 @jimbolla said that the naming in the reselect's docs is an established practice, so I'll leave the final say to @mxstbr.
Otherwise it should be good to go; I tested generated templates and didn't notice any issues.

[coveralls]

Coverage remained the same at 98.605% when pulling e659628 on Dattaya:selector-factories into 33485f4 on mxstbr:dev.

[kachkaev]

I'd stick to an established practice. New users of react-boilerplate will otherwise struggle because of a mismatch in naming. IMHO the goal of this boilerplate is to glue the best practices rather than reinventing the wheel :–)

[coveralls]

Coverage remained the same at 98.605% when pulling 8f3710a on Dattaya:selector-factories into f0eb229 on mxstbr:dev.

[Dattaya]

It would be nice to get two reviews from collaborators, because this one could've been merged long time ago.

[coveralls]

Coverage remained the same at 98.578% when pulling 7e8325b on Dattaya:selector-factories into 7652c6d on mxstbr:dev.

[coveralls]

Coverage remained the same at 98.578% when pulling 7e8325b on Dattaya:selector-factories into 7652c6d on mxstbr:dev.

[mxstbr]

Sorry for taking so long, this LGTM!

[mxstbr]

/cc @gihrig @jwinn @justingreenberg LGTY?

[jwinn]

I haven't pulled and tested locally, but the naming changes LGTM and seem inline with reselect conventions. 👍

[mxstbr]

Same, let's then!

Thanks @Dattaya!

[gihrig]

Late to the show, but, I discovered a bug...

As of this PR, a cleaned project fails on npm start

To reproduce:

git clone https://github.com/mxstbr/react-boilerplate.git react-bp && cd react-bp
git checkout 50fef95d9f2e589385174250e2855fbbdd6907fb
yarn <-- note that npm start will work as expected here.
npm run clean
npm start

In the terminal I get:

In the browser console:

[Dattaya]

Sorry about that, and thanks for the detailed report, @gihrig, more than enough information for me to fix it immediately.

[siberianguy]

I'm confused about this change. makeXXX syntax is recommended in reselect docs not as a default approach but as a solution to a specific problem. Do we really need to push it for our app that even doesn't have selectors depending on props (that where selector factories shine)?

[Dattaya]

@alexidsa, agreed, selector factories like makeSelectUsername make more sense as regular selectors. These too, because as you said we don't depend on props:

createStructuredSelector({
  repos: makeSelectRepos(),
  username: makeSelectUsername(),
  loading: makeSelectLoading(),
  error: makeSelectError(),
})

but @mxstbr probably wanted to bring more awareness to this interesting approach.

Note that they were recreated on each call before, I just renamed them in this PR, if you want, you can create a new issue for discussion, or event better, send a PR.

[siberianguy]

I agree that this approach is better than a previous one (before your PR we had factories without good naming).

agreed, selector factories like makeSelectUsername make more sense as regular selectors

What do you mean by that? My point was quite the opposite. I think factories make sense only when we actually need them (i. e. we depend on props) not as regular/ordinary selectors.

May be a good compromise would be use normal selectors as default but use factories in case when it's really needed. We need a component that is rendered multiple times for that. Repository component seems like a good option for that.

[Dattaya]

May be a good compromise would be use normal selectors as default but use factories in case when it's really needed.

Definitely! I don't know about other collaborators, but I completely support this idea.

[siberianguy]

I'm still getting used to the codebase but I'll try to make a PR in near future.

[tomasfrancisco]

I found an inconsistency between selectors.js.hbs and selectors.test.js.hbs.

Or the domain selector function should be named with

makeSelect{{ properCase name }}Domain

at selectors.js.hbs, or the import on respectively test should be

import { select{{ properCase name }}Domain } from '../selectors';

at selectors.test.js.hbs, instead of

import { makeSelect{{ properCase name }}Domain } from '../selectors';

at selectors.test.js.hbs.

What is the option that should be took?

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.