Repository navigation
Fetch data with yubikiri - #565
Conversation
|
@kuychaco @smashwilson This switches data fetching to use Yubikiri in all the controllers. Would love a quick review just to double check that I didn't mess something up. Also fixed a few linter errors and a couple odd test behaviors. |
|
|
||
| if (data.remoteName) { | ||
| data.aheadCount = await repository.getAheadCount(data.branchName); | ||
| data.behindCount = await repository.getBehindCount(this.branchName); |
There was a problem hiding this comment.
I maintained data.branchName being passed to getAheadCount and this.branchName being passed to getBehindCount though I'm not certain why the calls use different values
There was a problem hiding this comment.
Huh. Offhand I'm pretty sure that's a 🐛 and they should both be data.branchName. 🤔
smashwilson
left a comment
smashwilson
left a comment
There was a problem hiding this comment.
Only one comment that you might want to take action on. Other than that looks great 😄
|
|
||
| if (data.remoteName) { | ||
| data.aheadCount = await repository.getAheadCount(data.branchName); | ||
| data.behindCount = await repository.getBehindCount(this.branchName); |
There was a problem hiding this comment.
Huh. Offhand I'm pretty sure that's a 🐛 and they should both be data.branchName. 🤔
| aheadCount: async query => { | ||
| const remoteName = await query.remoteName; | ||
| return remoteName ? repository.getAheadCount(await query.branchName) : null; | ||
| }, |
| .filter(remote => remote.info.githubRepo); | ||
| return {remotes, currentBranch, selectedRemote}; | ||
| fetchData: repo => { | ||
| return yubikiri({ |
There was a problem hiding this comment.
Random thought: to be a little more declarative, it might be cool to have an alternative to fetchData that just accepts the yubikiri map directly.
There was a problem hiding this comment.
Hm, what would that look like? E.g. the values to the object would need to be repo.thing() but we have no repo
| commandRegistry={commandRegistry} | ||
| notificationManager={notificationManager} | ||
| confirm={confirm} | ||
| tooltips={tooltips} |
| view.mouseup(); | ||
| assertEqualSets(view.selection.getSelectedItems(), new Set(unstagedChanges.slice(0, 2))); | ||
| assert.equal(view.props.didSelectFilePath.callCount, 0); | ||
| view.element.remove(); |
| behindCount: async query => { | ||
| const remoteName = await query.remoteName; | ||
| return remoteName ? repository.getAheadCount(this.branchName) : null; | ||
| return remoteName ? repository.getAheadCount(await query.branchName) : null; |
There was a problem hiding this comment.
@kuychaco We're pretty sure this was a bug, let us know if we were wrong ^_^
This PR converts data fetching in components to use yubikri in order to more easily and correctly fetch data in parallel.