Summary:
The SVG was artisanally crafted by yours truly, and rasterized by the
accompanying script (which is fully deterministic, and also artisanal).
Test Plan:
Run `./src/assets/logo/rasterize.sh`, and note that the output is
unchanged.
wchargin-branch: add-logo
Summary:
This is a follow-up to #514, wherein we disabled new service workers and
instructed any existing service workers to self-destruct. (See that PR
for the rationale.) This commit removes them from our codebase entirely,
enabling us to slim down our build process and our build output.
Test Plan:
Running `yarn start` still works. Building the static site and exploring
it works, too.
wchargin-branch: remove-sw
Summary:
This fixes the following warning on our development instance:
> Warning: render(): Calling ReactDOM.render() to hydrate
> server-rendered markup will stop working in React v17. Replace the
> ReactDOM.render() call with ReactDOM.hydrate() if you want React to
> attach to the server HTML.
We do in fact want to attach to the server HTML, so we apply the
suggested patch.
(The warning of course also applies to production, but warnings do not
appear in production.)
Test Plan:
Running `yarn start` shows that the above warning has disappeared, and
that the cred explorer still works. (Also, `yarn test --full` passes,
but that tells us effectively nothing because this code path is never
hit in tests: it only affects the HTML that is executed in the browser.
Erasing the entire module, leaving only `// @flow`, still lets tests
pass.)
wchargin-branch: migrate-to-hydrate
This reverts commit 8c70f03122ed0de24ce7382dcb5fd098f199a9dd.
Context: This introduced a serious bug (#631), so we're reverting it to
get the codebase back in a working state. Meanwhile, I'll develop a
principled solution.
Test plan:
I rebuilt the backend, re-loaded a graph, and loaded it in the frontend.
PageRank, the cred explorer, and the weight config all work. Opening a
pull request does not trigger a crash.
This implements two methods:
`aggregateByNodeType` groups `scoredConnection`s by the specified
`NodeTypes`, along with summary statistics.
`aggregateByConnectionType` groups `scoredConnection`s by
`ConnectionType` at the top level, where `ConnectionType` includes
`EdgeType` and direction, (and also captures synthetic self-loops).
Then it also groups by `NodeType` within any aggregation.
This is progress towards #502.
Test plan: unit tests included.
See #627 for context.
Removing the Git plugin results in an enormous performance improvement.
In my testing on `metamask/metamask-extension`: before this change, load
took 23s, and PageRank took >9 mins and then crashed. After this change,
load+PageRank took 5s combined.
Test plan: All unit tests pass; loading new data from the CLI works; and
I poked around the UI to make sure there were no spurious references to
the Git plugin.
Note: This does not break backcompat, there's no need to regenerate any
already-loaded data.
This is the first real step towards #502.
Factoring this out because deciding the type signatures was non-trivial,
and the work was paired with @wchargin.
Test plan: `yarn test`
PagerankTable is getting a bit unwieldy, especially as #502 will need to
add a new pair of components. This commit splits the erstwise
PagerankTable.js into four files:
- `pagerankTable/shared`, shared utils and types
- `pagerankTable/Node`, the `NodeRow` and `NodeRowList`
- `pagerankTable/Connection`, the `ConnectionRow`, `ConnectionRowList`,
and `ConnectionView`
- `pagerankTable/Table`, the `PagerankTable` itself
This commit makes no logical changes; it is purely a reorganization.
Test plan: `yarn test`
PageRank outputs scores as components in a probability distribution.
This means that most scores are very small numbers, e.g. 0.00003. This
doesn't make for a great UI (humans don't like thinking in tiny
decimals).
Our first attempt to come up with a more readable UI was to use log
scores; in #265 we displayed the log score alongside (arbitrarily)
`rawScore * 100` in the UI. The log scores were more usable, so we kept
them, with subsequent modifications. In the original version, all the
log scores were negative. In #466, we arbitrarily added 10 to the
scores, which made most scores look nicer, but introduced a meaningless
switch where scores counter-intuitively become negative after a certain
point. That was bad, so in #535 we started displaying negative log
scores. This is also counter-intuitive: it's weird that lower scores are
better, and it's not clear that a score of (say) 3 is 20x better than a
score of 6.
I think we need to do away with the log scores; people just don't think
about numbers logarithmically. This commit switches to linear scores,
normalized so that the largest score is always 1000. I've tried this out
on a few repos and demo'd it to people, and it seems much clearer.
Test plan: Some unit tests added; also, I launched the cred explorer and
experienced the change on several projects.
We manually import the set of plugins to be used by the app in a few
different places. That's a bad smell. This commit creates a centralized
import point instead.
Test plan: `yarn test`
This commit adds some consistent and tested methods for getting the
appropriate plugin adapter for a given Node/Edge address. There are
methods for both static and dynamic adapters.
In the event that more than one plugin adapter matches the given
address, an error is thrown; likewise in the case where there is no
matching adapter.
Test plan: `yarn test`
Relevant to #465
Summary:
The `node ./bin/sourcecred.js load` command invokes plugin code by
providing an output directory into which the plugin may store data.
As of this patch, it also provides a cache directory that the plugin may
use to store data that will not be available at runtime. For instance,
the Git plugin might choose to clone the repository herein, or the
GitHub plugin may choose to store partial GraphQL query results to deal
with interruptions. The contract is that the cache directory may be
removed at any time and that the plugin should continue to operate
normally.
Test Plan:
The build script has been updated and tested. Reverting the change to
the build script causes the newly added test to fail. (Each plugin has a
cache directory, though the cache directories are empty for now.)
wchargin-branch: create-plugin-cache
This reverts commit 25b0124b56caaa93ff1c15fec20c1376ba609280.
@wchargin had an extensive offline conversation about PluginAdapters,
and decided that for now we awnt to punt on figuring out the right
abstraction for having a "core"-scoped plugin adapter. Instead, we'll
keep on using plugin adapters as something of a kitchen sink where we
throw in all the per-plugin logic that we need to run the app.
This necessitates reverting #615 because we don't think that React
should be a dependency in core, but we will need the
DynamicPluginAdapter to have a type dependency on React so that we can
solve issues like #590.
Test plan: Yarn test suffices.
There's no reason for it to be in `app` - the concepts it contains are
core concepts, e.g. node types and graphs.
For now I'm leaving the `NodeType` and `EdgeType` interfaces with the
PluginAdapter. They may move later, but I'm relucant to clutter the
graph class more, and all I need for now is that they live in core.
Test plan: It's just a move/rename, so `yarn test` is amply sufficient.
Summary:
We never use the `node ./bin/sourcecred.js start` command. This command
contains an Express server to combine the static files with the build
output, which duplicates the logic in our Webpack config, which we
actually use (with `yarn start`). Once we actually want the command line
entry point to be a useful tool for end users, we can consider
reimplementing it the right way, whatever that may be. Until then, it’s
simply one more thing to keep in sync.
Test Plan:
Running `yarn test --full` passes; the `load` CLI command still works;
running `yarn start` still works.
wchargin-branch: remove-start
For #465, I'm planning to create an abstraction over NodeTypes and
EdgeTypes which traverses a hierarchy of types and aggregates/reduces
information across all the matching types for a given Node/Edge address.
To do that efficiently, we will want tries[1].
Thanks to @wchargin for helping me figure out how to implement this.
Test plan: Unit tests. The code is a little tricky so please review it
closely.
[1]: https://en.wikipedia.org/wiki/Trie
Consider the following types:
```
// Used to be called "Contributor"
export type Adjacency =
| {|+type: "SYNTHETIC_LOOP"|}
| {|+type: "IN_EDGE", +edge: Edge|}
| {|+type: "OUT_EDGE", +edge: Edge|};
// Used to be called "Contribution"
export type Connection = {|
+adjacency: Adjacency,
// This `weight` is a conditional probability: given that you're at
// the source of this connection's adjacency, what's the
// probability that you travel along this connection to the target?
+weight: Probability,
|};
// Used to be called "ScoredContribution"
export type ScoredConnection = {|
+connection: Connection,
+source: NodeAddressT,
+sourceScore: number,
+connectionScore: number,
|};
```
These types represent how a node's PagerankScore is influenced by its
connections in the markov chain. The previous names, "Contributor",
"Contribution" and "ScoredContribution", were quite confusing as
elsewhere in the project "contributon" means something that added value
to the project, and "contributor" means the author of contributions.
While these new names aren't necessarily much better a priori, in the
context of the project's vernacular they are much less confusing.
Test plan: It's just a rename, and `yarn test` passes.
Historically, `credExplorer/App.js` instantiated a PagerankTable
regardless of its state, and would pass null props when the App didn't
have data needed to load the table. As of #583, we just don't create the
PagerankTable before its data is available, which is a simpler/better
contract. As such, the type signature of PagerankTable's props can be
simplified, and some logic for handling the null case may be removed.
Test plan: `yarn test` passes, which is sufficient.
Pull #579 reifies the cred explorer state as an explicit state
transition machine, with a well-tested implementation. This pull
re-writes `credExplorer/App.js` to use that implementation, and thoroughly
tests it.
The result is that `credExplorer/App.js` has much simpler code
(because it just binds the rendered components to the state machine),
and is much more thoroughly tested.
To ensure easy testability of the `App` class, it was refactored so that
the module exports a factory function which takes a method for creating
the `AppStateTransitionMachine` and returns an `App` class. This ensures
that in test code, we can easily mock the state transition machine. This
had no effect on external callers, since the higher-level `<AppPage>`
class, which already wraps over `LocalStore` choice, was already the
preferred call site.
I also added a loading indicator component, which presently displays a
status text corresponding to the state, such as "Loading graph...", or
"Error while running PageRank". This way, there is always some user
feedback during loading states (which could take a while).
Test plan:
Visual inspection, and the very thorough included unit tests.
Summary:
Here, we make the width consistent across the home page, the explorer,
and the navbar. Arguably, the PageRank table itself should be wider. We
can let it pop out of the box model with `relative`, `width`, and `left`
properties (using `calc`), but we don’t want to deal with the details
right now.
At some time in the future, we can also of course unify these styles.
Paired with @decentralion.
Test Plan:
Running `yarn start` and clicking around the various pages suffices. To
check the external redirect page, you can apply
```diff
diff --git a/src/app/HomePage.js b/src/app/HomePage.js
index 4d0f832..0eee519 100644
--- a/src/app/HomePage.js
+++ b/src/app/HomePage.js
@@ -143,7 +143,8 @@ export default class HomePage extends React.Component<{||}> {
<h2>Roadmap</h2>
<p>
SourceCred is under active development.{" "}
- <Link className={css(styles.link)} to="/prototype">
+ <Link className={css(styles.link)} to="/discord-invite">
+ {/* STOPSHIP */}
We have a prototype
</Link>{" "}
that ingests data from Git and GitHub, computes cred, and allows the
```
and then click the appropriate link on the home page.
wchargin-branch: max-width-900
Summary:
@decentralion and I have spent a bunch of time on this prose, and we
think that it’s pretty good. Let’s put some content on our otherwise
bare site.
Test Plan:
Running `yarn start` suffices.
Paired with @decentralion.
wchargin-branch: home-page-prose
The cred explorer app has a variety of valid states. Currently, it is
thrown together without explicit documentation of what its states are,
how they transition, or error handling or testing. I worry that this
will be hard to maintain.
This commit creates the AppState type which explicitly reifies every
reachable state for the app, and a StateTransitionMachine which handles
transitions between states. The transitions are thoroughly tested,
including edge cases where the user makes a change while waiting for a
promise to resolve, or where one of the promises failes.
Test plan:
The unit tests are comprehensive. `yarn test` passes.
Thanks to @wchargin for much discussion about how to structure the
states.
"Explore(r)" does not accurately convey the current state of the
project. In order to more accurately convey the current state,
"Explore(r)" has been updated to "Prototype"
Addresses #584
Test plan: Visual inspection and manual testing of pathing
Currently, the navbar lacks styling, and only provides links to the home and explorer pages.
This change adds:
* Better styling for the navbar
* External links to SourceCred github and twitter
Test plan: visual inspection
Summary:
This subtree has no effect on the new build process; it contains only
stale code.
Test Plan:
Running `yarn test --full` passes. Running `yarn build` and running an
HTTP server on the result indicates the expected behavior, as does
running `yarn start`. A quick `git grep public` finds no amok results.
wchargin-branch: remove-public
This commit removes some unecessary text and whitespace from the cred
explorer. The result is more minimal. It's not super pretty, but that
will come later.
Test plan: Visual inspection
This commit removes the node and edge counts that are displayed on the
cred explorer. While this is nice information to surface, I think it's
currently surfaced in the wrong place: it should be displayed as part of
the PagerankTable.
My proximate motivation for removing it is that it cleans up the data
structure slightly and I'm about to intensively refactor the whole file.
Test plan: `yarn test`; also, I manually engaged the cred explorer and
it still works properly.
Our data model orients on getting repos from GitHub, which are
alternatively represented as strings like "sourcecred/sourcecred", or
pairs of variables representing the owner and name, or objects with
owner and name properties. We also have a few different implementations
of repo validation, which are not applied consistently.
This commit changes all that. We now have a consistent Repo type which
is an object containing a string owner and string name. Thanks to a
clever suggestion by @wchargin, it is implemented as an opaque subtype
of an object containing those properties, so that the only valid way to
construct a Repo typed object is to use one of the functions that
consistently validates the repo.
As a fly-by fix, I noticed that there were some functions in the GitHub
query generation that didn't properly mark arguments as readOnly. I've
fixed these.
Test plan: No externally-observable behavior changes (except insofar as
there is a slight change in variable names in the GitHub graphql query,
which has also resulted in a snapshot diff). `yarn travis --full`
passes. `git grep repoOwner` presents no hits.
Test Plan:
None really needed—the infrastructure has already been tested—but you
can verify that this works both under `yarn start` and `yarn build` by
navigating to the evident URL.
wchargin-branch: discord-invite
Summary:
This patch extends our routing infrastructure to add support for
_external_ redirects. It does not include dedicated support for
site-internal redirects.
Test Plan:
Add an external redirect to `routeData`, like the following:
```diff
diff --git a/src/app/routeData.js b/src/app/routeData.js
index 83dff72..eaba130 100644
--- a/src/app/routeData.js
+++ b/src/app/routeData.js
@@ -36,6 +36,15 @@ const routeData /*: $ReadOnlyArray<RouteDatum> */ = [
title: "SourceCred explorer",
navTitle: "Explorer",
},
+ {
+ path: "/discord-invite",
+ contents: {
+ type: "EXTERNAL_REDIRECT",
+ redirectTo: "https://discord.gg/tsBTgc9",
+ },
+ title: "SourceCred Discord invite",
+ navTitle: null,
+ },
];
exports.routeData = routeData;
```
Then:
- run `yarn build`, and:
- verify that the appropriate `index.html` file is correctly
generated;
- verify that opening the `index.html` file in a browser redirects
you to the appropriate destination, even with JavaScript
disabled;
- verify that the link in the body of the HTML page is correct
(easier to do if you remove the `<meta>` tag)
- run `yarn start`, and:
1. use the React DevTools to change the “Explorer” link’s `to` prop
from `/explorer` to `/discord-invite`;
2. click the link; and
3. verify that you are properly redirected.
wchargin-branch: add-external-redirect
@wchargin suggested displaying scores this way. This way, lowest scores
are best, and higher scores are worse. This is a little
counterintuitive, but maybe less counterintuitive than the current
approach, which arbitrarily adds 10 to scores to keep them non-negative,
and results in an arbitrary crossing point where scores become negative
without any particular significance.
Test plan: Travis, and manual inspection of the frontend.
Summary:
In addition to a routine libdef update, we also need to work around a
particularly nasty new bug in Flow, which requires `any`-casts that are
even more unsafe than usual. That said, I think that it’s worth that
cost to remain up to date with Flow, so that we can amortize future such
issues.
Test Plan:
Running `yarn travis --full` passes.
wchargin-branch: upgrade-flow-v0.76.0
Some slight changes were needed to the other test code to avoid spurious
errors. Specifically, we now always set up a mocked fetch response in
non-failure cases, even if we don't wait for it to resolve.
Test plan: I manually tested it, also see the new unit tests.
Modifies the PluginAdapter interface so that NodeTypes come with
default weights, and modify the WeightConfig so that it loads those
NodeTypes as the default weights.
The new weight choices are not super principled, but are clearly better
than the uniform default. Now, projects find that most pull requests are
more valuable than most git blobs. :)
Sadly, the WeightConfig does not yet have any tests, so there are no
test changes.
Test plan: I manually verified that it works as expected, by clearing
application data and reloading the cred explorer.
Currently, the GitHub graph fetcher will characteristically fail if:
1. it times out GitHub's server
2. it triggers the semidocumented abuse detection mechanism
In case 1, an intelligible error is posted to the console. In case 2, it
produces an unintelligible TypeError, because the response is not a
valid GraphQL response (the error field is not populated; it has a
custom message instead).
As of this commit, we gracefully catch both cases, and print a message
to console directing the user to #350, which has context on GitHub query
failures. This new catch works because in case 2, the data field is
empty, so we now properly recognize `x.data === undefined` as an error
case.
Thanks to @wchargin for the investigatory work behind this commit.
Fixes#223.
Test plan:
We don't have unit tests that cover this case, but I did manually test
it by asking GitHub to fetch `ipfs/go-ipfs`, which consistently fails.
I also tested it by using an invalid length-40 GitHub API token.
Summary:
There have been a couple of occasions on which we’ve considered using
it, but didn’t want to require from `app/`.
Test Plan:
Unit tests added, with full coverage.
wchargin-branch: extract-dedent
In #529, I made the cred explorer populate a dropdown with the list of
repositories that are available to explore. That dropdown defaults to
selecting the alphabetically first repository.
This has an unfortunate consequence in that it makes it impossible for
us to explicitly set a default - for example, we would like
sourcecred.github.io/explorer to show sourcecred/sourcecred by default,
but instead it shows example-git.
So that we can choose the default, I've changed the logic so that it
instead shows the most-recently-loaded data first. This required
a breaking change to the repoRegistry serialized format, so I've also
refactored the module to use compat, which I should have done from the
beginning.
Test plan:
Unit tests for the repo selector are updated. The CLI load command
unfortunately has no tests, so I manually tested that it always provides
the lastest repository last, and appropriately handles the case where
the same repository is loaded multiple times.
Context: The Cred Explorer loads data (currently on a per-repository
basis) that has previously been prepared by running the `sourcecred
load` cli command.
Currently, to select a repository to load, the user must manually type
the repository owner and name. This is a confusing UI, because it
suggests that any repository may be chosen, when in fact only repos
already loaded into the data store are available. The user is given no
feedback as to which repositories are valid options.
As of #516, the backend stores a registry listing available
repositories. This commit adds a `RepositorySelect` component which
loads the available from that registry, and makes them available in a
dropdown, in sorted order.
When the user manually selects one of the repositories, that selection
is persisted into localStorage and respected on future loads. If the
user hasn't made such a choice, then the first repository is selected by
default.
The implementation is highly influenced by testability considerations.
The default export, `<RepositorySelect onChange={} localStore={} />`, is
pretty straightforward. The `RepositorySelect` is somewhat cumbersome to
test because it asynchronously fetches data and then updates its state,
which affects the render output. So as to avoid testing inside async
react components wherever possible, I've factored out:
* `loadStatus`, which uses fetch and localStore to get the status of the
selector.
* `PureRepositorySelect`, which just renders a `Status`, such as
loading, failure, or valid
* `LocalStoreRepositorySelect`, which wraps the `PureRepositorySelect`
with logic to bind the repository select to localStore on change.
Test plan: Extensive unit tests were added. Also, to ensure that the
tests were testing the right thing, I manually tested:
- attempting to load invalid registry
- attempting to load with no registry
- attempting to load with empty registry
- loading without valid localStore
- changing the setting via dropdown
- loading from localStore after changing the dropdown
And all behavior was as expected.
Thanks to @wchargin for considerable help testing this PR.
Summary:
Test code should probably always use a checked, memory-backed local
storage implementation. This endpoint will help users not forget to
include the checks.
wchargin-branch: test-local-store
Summary:
Might as well have runtime type safety, in case we accidentally try to
store any more `Map`s or `undefined`s.
Test Plan:
Tests pass, but are likely not sufficient. Manual testing indicates that
the local storage still works, for both reads and writes, on a fresh
profile or with existing data, for both the repository owner/name and
the weight configuration.
wchargin-branch: use-checked-local-store
Summary:
We can use this in tests. If need be, we can enhance this class to allow
simulating failures, low storage limits, etc., but just having a pure
implementation at all is all we need right now.
Test Plan:
Unit tests added.
wchargin-branch: memory-local-store
