Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Fix newError noops when not bound to dispacher #4013

Merged
merged 6 commits into from
Jan 3, 2017
Merged

Conversation

jacogr
Copy link
Contributor

@jacogr jacogr commented Jan 3, 2017

newError in AddContract, EditMeta & PasswordManager was not properly bound.

@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Jan 3, 2017
}

export default connect(
mapStateToProps,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is just an optimisation:

If you omit [mapDispatchToProps], the component will not be subscribed to the Redux store.

You could pass null, it occurs multiple times in the codebase already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I generally prefer being explicit and in a once-off situation being explicit about an empty state is not bad. However who knows what happens with Redux making that call each cycle as things changes, it just add unnecessary overheads.

Will update.


describe('onAdd', () => {
it('calls store addContract', () => {
sinon.stub(instance.store, 'addContract').resolves(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think in this case it's not worth it yet, but once we have more stubs, restoring them can easily be forgotten (which breaks the whole point of the stubs then). sandboxes try to address this issue.

@@ -138,6 +142,24 @@ export default class EditMeta extends Component {

return this.store
.save()
.then(() => this.props.onClose());
.then(() => this.props.onClose())
.catch((error) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

}

export default connect(
mapStateToProps,
Copy link
Contributor

Choose a reason for hiding this comment

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

As above.

@derhuerst derhuerst added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 3, 2017
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling f0cc5a3 on jg-newError-bindings into ** on master**.

@jacogr jacogr merged commit 04ed53e into master Jan 3, 2017
@jacogr jacogr deleted the jg-newError-bindings branch January 3, 2017 16:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants