When using Tries, we often want the last matching entry for the given
path, and to throw an error if one is not available. By adding this
method to the API, we avoid a lot of unnecessary repetition in the code
base.
Test plan: Unit tests pass. As this touches the untested WeightConfig,
I've also manually tested the weight config behavior.
Summary:
An `import *` was used for convenience, but this effects a value import
in addition to a type import. By exploding the wildcard import to
directly import the required types, we can shave off 2.3% of our
post-gzip bundle size (131.82 KB to 128.74 KB). It’s unfortunate that we
lose the namespacing, but c’est la vie.
Test Plan:
`yarn flow` suffices.
wchargin-branch: explode-wildcard-type-import
Thanks to #642, it should now be safe to disable the Git plugin, reaping
the benefits described in #628, without causing the cred explorer to
crash (#631).
Test plan:
- `yarn travis --full` passes
- The full cred explorer works:
- Running PageRank does not crash the explorer
- Expanding a pull request does not crash the explorer
- (After clearing state) the weight config doesn't show Git weights
- The filter doesn't show Git nodes
This modifies WeightConfig to properly use the fallback node type, as
created in #640 and merged in #642. As an additional change, it now
displays type names, rather than the address parts. For example, the
issue type is now displayed as `Issue`, not
`["sourcecred", "github", "issue"]`.
The WeightConfig is an untested mess, and I will likely re-write it
entirely. (See a bevy of WeightConfig related issues: #604, #595, #588).
So, not too much effort was invested in keeping high code quality in
this commit.
Test plan: The weight config has no tests, so I manually tested:
- weights persist after page reload
- node weights influence cred attribution
- edge weights influence cred attribution
- edge directionality influences cred attribution
- weights have reasonably pretty description messages
This takes the code from #640 and puts it into production.
Test plan: Unit tests pass. The observable behavior in the cred explorer
is unchanged; i.e. the addition of the FallbackAdapter did not produce
new entries in the WeightConfig or in the Pagerank table options. The
WeightConfig is untested, so we don't have verification of that behavior
(other than that I tested it and am reporting it here). The
PagerankTable code is tested, and a snapshot would fail if another
option group had appeared.
Issue #631 revealed that our current plugin-handling code is fragile -
we aren't robust to having nodes from a plugin without having that
plugin present in the frontend. This commit adds `StaticAdapterSet` and
`DynamicAdapterSet`, which are abstractions over finding the matching
plugin adapter or type for a node or edge. It's a robust abstraction,
because the adapter sets always include the `StaticFallbackAdapter` or
`DynamicFallbackAdapter`, which can match any node, so we'll never get
an error like #631 due to not having an adapter / type available.
Also relevant: #465
Test plan:
Unit tests included.
Summary:
This patch considers an environment variable `GITHUB_DELAY_MS`. If the
value is set to a positive integer, a delay of the given number of
milliseconds will be incurred before each query to GitHub. This is to
decrease the probability of being rate-limited; see #350 for details.
This in turn unblocks us to load SourceCred data for larger
repositories.
The use of an environment variable is something of a hack to get this
off the ground. See #638 for long-term plans.
Test Plan:
Run `time node ./bin/sourcecred.js load sourcecred/example-github` with
varying values for `GITHUB_DELAY_MS`. Note that with the variable unset,
set to zero, set to a negative number, or set to a non-numeric value,
the job completes quickly; when the delay is set to `5000`, the job
takes an extra five seconds.
wchargin-branch: delay-github-queries
For #502: The UI that I currently have in mind displays aggregations
grouped by connection type and node type together, rather than nested.
I think it will be cumbersome to have multiple hierarchical
levels of expansion.
To make that UI easy to write, this commit adds some logic for
flattening the hiearchical aggregation from #624. I add an extra
translation to flatten, rather than just having the logic produce nested
structures, because it's convenient to keep around the nested structure
in case I decide to implement the hierarchical UI instead. Once we have
solidified how we want the UI to behave, we might choose to simplify
this code.
Test plan: The implementation is rather simple. There are some unit
tests.
This commit creates the directory `src/app/adapters` and moves the
following three files into it:
- `src/app/pluginAdapter.js`
- `src/app/pluginAdapter.test.js`
- `src/app/defaultPlugins.js`
This is in preparation for a principled fix for #631, which will add a
base plugin and some logic that ensures it's always included.
Summary:
In addition to the obvious benefit of having a favicon, this gets rid of
a 404 Not Found error on our home page, tremendously boosting our hacker
cred.
Test Plan:
The favicon is displayed in both `yarn start` and the static site (as a
result of the build script). The added build test fails before this
change.
wchargin-branch: add-favicon
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 8c70f03122.
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
It's annoying that it always appends it to the end of the PR message
when we've uploaded a single commit. Since we have new contributors
relatively rarely, but we are slightly annoyed by it constantly, it's
better to remove it.
Test plan: not needed
This reverts commit 25b0124b56.
@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 plan to allow plugins to store permanent data in `$SC/data/` and
temporary, ephemeral, or intermediate data in `$SC/cache/`. The latter
subtree will be excluded from the static site at build time, so it
behooves us to also exclude it from the development environment.
Test Plan:
Run `yarn start`. Then,
```shell
$ root='localhost:8080/api/v1/data'
$ curl -sI "${root}/repositoryRegistry.json" | head -1
HTTP/1.1 200 OK
$ curl -sI "${root}/data/sourcecred/example-git/github/view.json" | head -1
HTTP/1.1 200 OK
$ curl -sI "${root}/cache" | head -1
HTTP/1.1 400 Bad Request
$ curl -sI "${root}/cache/" | head -1
HTTP/1.1 400 Bad Request
$ curl -sI "${root}/cache/foo" | head -1
HTTP/1.1 400 Bad Request
$ curl -sI "${root}/cache/foo/bar/baz" | head -1
HTTP/1.1 400 Bad Request
```
Also, check that the app still works.
wchargin-branch: exclude-cache-from-dev-server
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
Summary:
This is a minimal patch to make the deploy script depend on the
separate, well-tested build script introduced in #592. The patch
suggests a number of improvements that can be made to the deploy script
itself, but I’m deferring those as they’re not on the critical path.
These are tracked at #610.
Test Plan:
Apply the below patch. Then run `./scripts/deploy.sh -n`, and verify
that the served output is correct, including the CNAME file. Verify that
when the `DEPLOY_CNAME_URL` default value is removed, the resulting
output does _not_ have a CNAME file.
Patch, for ease of building:
```diff
diff --git a/scripts/deploy.sh b/scripts/deploy.sh
index cddcece..8925e5b 100755
--- a/scripts/deploy.sh
+++ b/scripts/deploy.sh
@@ -23,7 +23,7 @@ main() {
preview_dir=
trap cleanup EXIT
- ensure_clean_working_tree
+ #ensure_clean_working_tree
build_and_deploy
}
@@ -76,8 +76,7 @@ build_and_deploy() {
"${sourcecred_repo}/scripts/build_static_site.sh" \
--target "${static_site}" \
${DEPLOY_CNAME_URL:+--cname "${DEPLOY_CNAME_URL}"} \
- --repo ipfs/js-ipfs \
- --repo sourcecred/sourcecred \
+ --repo sourcecred/example-github \
;
sourcecred_site="$(mktemp -d --suffix ".sourcecred-site")"
```
wchargin-branch: deploy-via-build
Summary:
Currently, we create the static site and deploy it all at once in
`scripts/deploy.sh`. This commit creates a new script that only builds
the static site. This has the advantage that it is easier/less scary to
change that script (because it can be tested without worrying about
deploying to a local test target), and that we can write automated tests
for it.
Test Plan:
Run `yarn sharness`; note that it completes very quickly. Then, in a
shell with your GitHub token exported, run `yarn sharness-full`. Expect
all tests to pass.
For a sanity check, you can run:
```shell
outdir="$(mktemp -d --suffix .sourcecred-site)"
./scripts/build_static_site.sh --target "${outdir}" \
--cname sourcecred.io \
--repo sourcecred/example-git \
--repo sourcecred/example-github \
;
(cd "${outdir}" && python -m SimpleHTTPServer)
```
and ensure that <http://localhost:8000/> is as expected.
One test case that is not covered is the following: _if_ the actual app
somehow tries to emit a `CNAME` file at root, _and_ our script’s logic
to catch this is broken, then we will not catch this failure. I’ve
tested the logic manually by adding `>"${cname_file}"` after definition
of that variable, but I don’t see a good way to test it automatically,
without adding flags like `--but-actually-emit-cname-too` to the build.
The compound probability of this happening is sufficiently low that this
doesn’t bother me.
wchargin-branch: build-static-site-script
Summary:
We will shortly want to perform testing of shell scripts; it makes the
most sense to do so via the shell. We could roll our own testing
framework, but it makes more sense to use an existing one. By choosing
Sharness, we’re in good company: `go-ipfs` and `go-multihash` use it as
well, and it’s derived from Git’s testing library. I like it a lot.
For now, we need a dummy test file; our test runner will fail if there
are no tests to run. As soon as we have a real test, we can remove this.
This commit was generated by following the “per-project installation”
instructions at https://github.com/chriscool/sharness, and by
additionally including that repository’s `COPYING` file as
`SHARNESS_LICENSE`, with a header prepended. I considered instead adding
Sharness as a submodule, which is supported and has clear advantages
(e.g., you can update the thing), but opted to avoid the complexity of
submodules for now.
Test Plan:
Create the following tests in the `sharness` directory:
```shell
$ cat sharness/good.t
#!/bin/sh
test_description='demo of passing tests'
. ./sharness.sh
test_expect_success "look at me go" true
test_expect_success EXPENSIVE "this may take a while" 'sleep 2'
test_done
# vim: ft=sh
$ cat sharness/bad.t
#!/bin/sh
test_description='demo of failing tests'
. ./sharness.sh
test_expect_success "I don't feel so good" false
test_done
# vim: ft=sh
```
Note that `yarn sharness` and `yarn test` fail appropriately. Note that
`yarn sharness-full` fails appropriately after taking two extra seconds,
and `yarn test --full` runs the latter. Each failure message should
print the name of the failing test case, not just the suite name, and
should indicate that the passing tests passed.
Then, remove `sharness/bad.t`, and note that the above commands all
pass, with the `--full` variants still taking longer.
Finally, remove `sharness/good.t`, and note that the above commands all
pass (and all pass quickly).
wchargin-branch: add-sharness
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.
Summary:
The information currently in the README is helpful but incomplete. We
now have a `CONTRIBUTING.md` guide, so we can simply link to that.
Test Plan:
None.
wchargin-branch: contributing-md-readme
"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:
Having a guide for contributors makes the experience more pleasant for
everyone. In the best case, contributors read the guide and submit
perfect PRs. In the event that a discrepancy occurs, we can gently point
the contributor to the guide—saving us from having to re-explain common
pitfalls, and signaling to the contributor that this is a matter of
practice and that we are not “out to get them”.
This guide, like all documentation, is intended to evolve over time.
Test Plan:
None.
wchargin-branch: contributing-md
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
Summary:
We were asking the `clean-webpack-plugin` to remove the `build/`
directory in all cases. However, Webpack accepts a command-line
parameter `--output-path`. When such a parameter is passed, we would be
removing the wrong directory.
The proper behavior is to remove “whatever the actual output path is”.
Webpack exposes this information, but it appears that the
`clean-webpack-plugin` does not take advantage of it. Therefore, this
commit includes a small Webpack plugin to do the right thing.
Test Plan:
Test that the behavior is correct when no output directory is specified:
```
mkdir -p build && touch build/wat && yarn build && ! [ -e build/wat ]
```
Test that the behavior is correct with an explicit `--output-path`:
```
outdir="$(mktemp -d)" && touch "${outdir}/wat" && \
yarn build --output-path "${outdir}" && \
! [ -e "${outdir}/wat" ]
```
Test that the plugin refuses to remove the root directory:
```
! yarn build --output-path . && \
sed -i '/path: /d' config/makeWebpackConfig.js && ! yarn build
```
(Feel free to comment out the actual `rimraf.sync` line in the plugin
when testing this.)
wchargin-branch: clean-actual-build-directory
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.
Summary:
This way, we can document what STOPSHIPs are without having to tip-toe
around the phrase.
Test Plan:
Add `STOPSHIP` to `README.md`, and note that `yarn test` fails before
this change but passes after it.
wchargin-branch: stopship-markdown-okay
Dandelion Mané
William Chargin
Claire L