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

State: Extract selectors to a global selectors file #720

Merged
merged 5 commits into from
May 9, 2017

Conversation

youknowriad
Copy link
Contributor

closes #693

To factorise some selectors and test them easily. I've moved all the redux selectors into a separate file and add unit tests.

@youknowriad youknowriad added the Framework Issues related to broader framework topics, especially as it relates to javascript label May 9, 2017
@youknowriad youknowriad self-assigned this May 9, 2017
@youknowriad youknowriad requested a review from nylen May 9, 2017 18:00
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Selectors are a thing we consciously avoided by worry about boilerplate and indirection. I like that you've taken a mild approach with a single selectors.js file. Testing selectors seems very valuable. I'd be okay with this, but curious to hear if others have concerns.

blocks: getBlocks( state ),
isRequesting: isSavingPost( state ),
isSuccessful: isPostSaveRequestSuccessful( state ),
isError: !! isPostSaveRequestFailed( state ),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can assume isPostSaveRequestFailed will return a boolean and drop the coercion

@nylen
Copy link
Member

nylen commented May 9, 2017

Selectors are a thing we consciously avoided by worry about boilerplate and indirection.

To address this, how about we make a rule (or rule of thumb) that we only extract something into a selector if it's more complicated than just a simple pull from state? For example, this doesn't seem to add much value:

export function isEditedPostDirty( state ) {
	return state.editor.dirty;
}

@aduth
Copy link
Member

aduth commented May 9, 2017

There is some value in treating state as an opaque shape with selectors as its isolated entry points; specifically that it leaves open the option to restructure the state object without requiring sweeping changes.

Copy link
Member

@nylen nylen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a clear preference either way. Also, even if some of the tests are trivial now, this won't always be the case, and the additional testability will serve us well for any future refactoring of the state tree.

I'll rebase and merge this 👍

@nylen nylen force-pushed the add/redux-selectors branch from be6a674 to 3f4a468 Compare May 9, 2017 22:02
@nylen nylen merged commit db94589 into master May 9, 2017
@nylen nylen deleted the add/redux-selectors branch May 9, 2017 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider adding Redux state selectors
3 participants