Skip to content

Commit

Permalink
GetNodes: Shuffle around to return an array of nodes
Browse files Browse the repository at this point in the history
This lets us use initial slugs that may map to multiple nodes (e.g. a
per-repo slug) instead of the old keys which had to be one-to-one with
nodes (e.g. a per-issue slug).

Also some test shenanigans to get everything passing with 100%
coverage.  For reasons I don't understand, my attempt at atomic state
pivots are still not sticking, which is why we still can't throw an
error in the duplicate-dummy-requests-for-a-single-key case.  I'm
suspicious of [1].  But a change at duplicate requests isn't the end
of the world either.

[1]: facebook/react#6895
  • Loading branch information
wking committed Nov 28, 2016
1 parent d730109 commit 99ff75e
Show file tree
Hide file tree
Showing 12 changed files with 449 additions and 310 deletions.
88 changes: 88 additions & 0 deletions webapp/__mocks__/github-api.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
class GitHub {
getIssues(user, repo) {
return new Issues(user, repo);
}

search() {
return new Search();
}
}

class Issues {
constructor(user, repo) {
this._user = user;
this._repo = repo;
this._calls = {};
}

_getIssue(number) {
if (this._calls[number]) {
throw new Error(
'duplicate call for github.com/' +
this._user + '/' + this._repo + '#' + number
);
}
this._calls[number] = true;
if (number > 100) {
return Promise.reject(new Error(
'error making request GET https://api.github.com/repos/' +
this._user + '/' + this._repo + '/issues/' + number
));
}

var dependencies = function (number) {
switch (number) {
case 1:
return ['#10'];
case 3:
return ['#2', 'd3/d3#4356', 'gitlab.com/foo/bar#234'];
case 5:
return ['#3'];
case 7:
return ['#3'];
case 10:
return ['#3', '#7', '#5'];
case 20:
return ['foo/#3']; /* will be skipped */
default:
return [];
}
};

return {
body: dependencies(number).map(function (dep) {
return 'depends on ' + dep;
}).join('\n') + '\n',
html_url: `https://github.com/${this._user}/${this._repo}/issues/${number}`,
number: number,
repository_url: `https://api.github.com/repos/${this._user}/${this._repo}`,
state: number < 10 ? 'open' : 'closed',
title: 'Some title for ' + number,
user: {
login: 'author' + number,
},
};
}

getIssue(number) {
return Promise.resolve({data: this._getIssue(number)});
}

listIssues() {
return Promise.resolve({
data: [
this._getIssue(1),
this._getIssue(2),
]
});
}
}

export class Search {
forIssues(options) {
var issues = new Issues('jbenet', 'depviz');
return issues.listIssues();
}
}

export default GitHub;
50 changes: 13 additions & 37 deletions webapp/src/App.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,52 +2,32 @@ import React, { Component } from 'react';
import { Router, IndexRoute, Route, hashHistory } from 'react-router'
import './App.css';
import DepGraph from './DepGraph';
import GetDummyHostNode, { CanonicalDummyHostKey } from './DummyHost';
import GetGitHubNode, { CanonicalGitHubKey, GetGitHubRepoNodes } from './GitHub';
import GetNode, { Canonicalizers, Getters, CanonicalKey } from './GetNode';
import GetDummyHostNodes, { CanonicalDummyHostKey } from './DummyHost';
import GetGitHubNodes, { CanonicalGitHubKey } from './GitHub';
import GetNodes, { Canonicalizers, Getters, CanonicalKey } from './GetNodes';
import Home from './Home';
import Layout, { HeaderHeight } from './Layout';

Canonicalizers['dummy'] = CanonicalDummyHostKey;
var dummyGetter = new GetDummyHostNodes();
Getters['dummy'] = dummyGetter.GetNodes.bind(dummyGetter);

Canonicalizers['github.com'] = CanonicalGitHubKey;
Getters['github.com'] = GetGitHubNodes;

export class DepGraphView extends Component {
render() {
var attributes = {};
const key = this.props.params.splat;
const host = key.split('/', 1)[0];
if (host === 'github.com') {
var match = /^github.com\/([^\/#]+)\/([^\/#]+)?$/.exec(key);
if (match === null) {
attributes.slugs = [key];
} else {
var user = match[1];
var repo = match[2];
attributes.getInitialNodes = function (pushNodes) {
return GetGitHubRepoNodes(user, repo, pushNodes);
};
}
} else {
attributes.slugs = [key];
}
return <DepGraph
width={window.innerWidth}
height={window.innerHeight - HeaderHeight}
getNode={GetNode} canonicalKey={CanonicalKey}
{...attributes} />
getNodes={GetNodes} canonicalKey={CanonicalKey}
slugs={[this.props.params.splat]} />
}
}

function enterGraphView(nextState, replace) {
const splat = nextState.params.splat;
var canonicalKey;
try {
canonicalKey = CanonicalKey(splat);
} catch (error) {
var match;
match = /^github.com\/([^\/#]+)\/([^\/#]+)?$/.exec(splat);
if (match === null) {
throw error;
}
canonicalKey = splat;
}
const canonicalKey = CanonicalKey(splat);
const canonicalPath = canonicalKey.replace(/#/g, '/');
if (splat === canonicalKey || splat === canonicalPath) {
return;
Expand All @@ -57,10 +37,6 @@ function enterGraphView(nextState, replace) {

class App extends Component {
render() {
Canonicalizers['dummy'] = CanonicalDummyHostKey;
Canonicalizers['github.com'] = CanonicalGitHubKey;
Getters['dummy'] = GetDummyHostNode;
Getters['github.com'] = GetGitHubNode;
return (
<div className="App">
<Router history={hashHistory}>
Expand Down
56 changes: 20 additions & 36 deletions webapp/src/App.test.js
Original file line number Diff line number Diff line change
@@ -1,35 +1,3 @@
jest.mock('github-api', () => {
class GitHub {
getIssues(user, repo) {
return new Issues(user, repo);
}
}

class Issues {
constructor(user, repo) {
this._user = user;
this._repo = repo;
}

getIssue(number) {
return Promise.resolve({
data: {
body: '',
html_url: 'https://github.com/' +
this._user + '/' + this._repo + '/issues/' + number,
state: number < 10 ? 'open' : 'closed',
title: 'Some title for ' + number,
user: {
login: 'author' + number,
},
}
});
}
}

return jest.fn(() => new GitHub());
});

import React from 'react';
import ReactDOM from 'react-dom';
import { hashHistory } from 'react-router'
Expand All @@ -40,10 +8,10 @@ it('home page renders without crashing', () => {
ReactDOM.render(<App />, div);
});

it('DepGraphView renders without crashing', () => {
it('issue view renders without crashing', () => {
const div = document.createElement('div');
ReactDOM.render(
<DepGraphView params={{'splat': 'dummy/jbenet/depviz/1'}} />,
<DepGraphView params={{'splat': 'dummy/jbenet/depviz/30'}} />,
div
);
});
Expand All @@ -54,10 +22,26 @@ it('entering graph view normalizes non-canonical paths', () => {
<App />,
div,
function () {
hashHistory.push('/http/dummy/jbenet/depviz/issues/1');
hashHistory.push('/http/dummy/jbenet/depviz/issues/31');
expect(
hashHistory.getCurrentLocation().pathname
).toBe('/http/dummy/jbenet/depviz/1');
).toBe('/http/dummy/jbenet/depviz/31');
},
);
});

it('repo view renders without crashing', () => {
const div = document.createElement('div');
ReactDOM.render(
<DepGraphView params={{'splat': 'github.com/jbenet/depviz'}} />,
div
);
});

it('user view renders without crashing', () => {
const div = document.createElement('div');
ReactDOM.render(
<DepGraphView params={{'splat': 'github.com/jbenet'}} />,
div
);
});
28 changes: 10 additions & 18 deletions webapp/src/DepGraph.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,22 +13,19 @@ class DepGraph extends PureComponent {

componentDidMount() {
this.nodes();
if (this.props.getInitialNodes) {
this.props.getInitialNodes(this.pushNodes.bind(this));
}
}

nodes() {
if (!this.props.getNode) {
throw new Error('getNode unset');
if (!this.props.getNodes) {
throw new Error('getNodes unset');
}
if (!this.props.canonicalKey) {
throw new Error('canonicalKey unset');
}
for (var index in (this.props.slugs || [])) {
for (var index in this.props.slugs) {
if (true) {
var key = this.props.slugs[index];
this.getNode(key);
this.getNodes(key);
}
}
}
Expand Down Expand Up @@ -56,24 +53,22 @@ class DepGraph extends PureComponent {
for (var i in parents) {
if (true) {
var relatedKey = parents[i];
this.getNode(relatedKey);
this.getNodes(relatedKey);
}
}
}
}
}
}

getNode(key) {
getNodes(key) {
var _this = this;
key = this.props.canonicalKey(key);
this.setState(function (prevState) {
if (prevState.nodes[key] || prevState.pending[key]) {
return prevState;
}
_this.props.getNode(key).then(function (node) {
_this.pushNodes([node]);
});
_this.props.getNodes(key, _this.pushNodes.bind(_this));
var pending = {...prevState.pending};
pending[key] = true;
return {...prevState, pending: pending};
Expand All @@ -82,17 +77,14 @@ class DepGraph extends PureComponent {

/* Properties:
*
* * slugs, (optional) roots for the issue graph. An array of
* strings, like:
* * slugs, roots for the issue graph. An array of strings, like:
* ['github.com/jbenet/depviz#1', 'gitlab.com/foo/bar#123']
* * getInitialNodes(pushNodes), (optional) a callback for resolving
* initial nodes. pushNodes takes an array of nodes and may be
* called multiple times.
* * width, the width of the graph viewport in pixels.
* * height, the height of the graph viewport in pixels.
* * canonicalKey(key) -> key, a callback for canonicalizing node
* names.
* * getNode(key) -> Node, a callback for resolving nodes.
* * getNodes(key, pushNodes) -> [Node, ...], a callback for
* resolving nodes.
*/
render() {
var _this = this;
Expand Down
Loading

0 comments on commit 99ff75e

Please sign in to comment.