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
Summary:
By pre-filling the “summary” and “test plan” fields, we create a strong
suggestion that people fill them out.
I’ve written this template in accordance with Phabricator commit style
(which is also what I use personally). I don’t intend for this to be
normative. It is perfectly valid to leave off the “Summary” header, for
instance, and it is perfectly valid to spell “Test Plan” with a
lowercase “p”. However, for a template in particular I think that
including the “Summary” header is better than excluding it, to give
contributors an idea of what’s supposed to go there.
I expect that upon drafting a pull request with a single commit, the
commit’s body, if non-empty, will take precedence over the pull request
template. If this is not the case, then we can reconsider whether we
want to include this template.
Test Plan:
Note that [GitHub documentation][1] indicates that this is the correct
file path. Then, merge and hope for the best.
[1]: https://blog.github.com/2016-02-17-issue-and-pull-request-templates/
wchargin-branch: pull-request-template
Summary:
Running `yarn test` (equiv. `npm test` or `npm run test`) now runs all
checks. It takes the place of the former `yarn travis`. This is more in
line with the expectation of a top-level `test` command: if it passes,
your code is good.
The `unit` command now runs Jest once, not in watch mode. It takes the
place of the former `ci-test`. To run tests in watch mode, run any of
the following:
- `yarn unit --watch`, or
- `npm run unit -- --watch`, or
- `npm unit -- --watch`.
This behavior is more consistent with the standard behavior of commands
like `make test`. It is also empirically what @wchargin and
@decentralion want most of the time.
Test Plan:
Verify that each of the scripts `test`, `unit`, and `coverage` passes.
Verify that each of the aforementioned `--watch` invocations works.
Verify that `.travis.yml` has the correct `script:` command.
wchargin-branch: reorganize-test-command
Test Plan:
Run `yarn start` and note that everything checks out.
Run `yarn build && (cd build/ && python -m SimpleHTTPServer)` and note
that everything checks out, except that the static assets are of course
not included in the build.
wchargin-branch: webpack-replace-scripts
Summary:
In our current system, we build by invoking `scripts/build.js`, which
begins by removing the `build/` directory. This behavior is nice,
because it prevents cross-contamination between builds. In this commit,
we add a plugin to achieve the same result from directly within Webpack.
Test Plan:
Run
```
mkdir -p ./build
touch ./build/wat
NODE_ENV=production node ./node_modules/.bin/webpack \
--config config/makeWebpackConfig.js
```
and ensure that `./build/wat` does not exist after the build completes.
wchargin-branch: webpack-clean-build
Summary:
This commit makes the Webpack dev server fully functional under the new
config, by serving the static SourceCred directory via a piece of
injected middleware.
Test Plan:
Run
```
NODE_ENV=development node ./node_modules/.bin/webpack-dev-server \
--config config/makeWebpackConfig.js
```
and navigate to the cred explorer. Note that the repository registry is
fetched, and the whole cred explorer works.
wchargin-branch: webpack-statics
Summary:
Extraction of the plugin list to a function is mostly trivial, but
requires a novel `// $ExpectFlowError`. The error has been there the
whole time, but Flow only catches it now. Why? Who knows.
Test Plan:
Run
```
NODE_ENV=development node ./node_modules/.bin/webpack-dev-server \
--config config/makeWebpackConfig.js
```
Note that the compilation/recompilation time is much faster than
previously.
wchargin-branch: webpack-minify-prod-only
Summary:
In addition to simply disabling the prod-only check, we apply a
workaround for a known bug that breaks static site generation in Webpack
versions >= 2.0.
Test Plan:
Run
```
NODE_ENV=development node ./node_modules/.bin/webpack-dev-server \
--config config/makeWebpackConfig.js
```
and visit http://localhost:8080/webpack-dev-server/ (note the trailing
slash) or just http://localhost:8080/. Expect the server to be slow, as
it is actually building for production.
wchargin-branch: webpack-enable-dev
Summary:
This will enable us to differentiate the production and development
behavior where necessary (primarily, only running minification in prod).
Best reviewed with `git show -w`.
Test Plan:
The diff with `git show -w` and the fact that `yarn flow` passes should
be sufficient. If you really want to be thorough, run Webpack with this
config file and `NODE_ENV` set to `production`.
wchargin-branch: webpack-functionize
Summary:
There really should be an `// $ExpectFlowError` on the dynamic `require`
on line 182:
```js
paths: require(paths.appRouteData).routeData.map(({path}) => path),
```
However, for some reason Flow does not catch this error now, so adding a
suppression comment generates an “unused suppression” warning. We
therefore omit the suppression in this commit; we will add it later,
once Flow magically finds the error.
Test Plan:
`yarn flow` reports no errors; a deliberately introduced error is
properly caught.
wchargin-branch: webpack-flow
Summary:
This module will become the shared home of the production and
development configurations.
Test Plan:
Run:
```
rm -r build/
NODE_ENV=production node node_modules/.bin/webpack \
--config config/makeWebpackConfig.js
(cd build && python -m SimpleHTTPServer)
```
and load http://localhost:8000. Note that the main content of app still
works, although the static assets in the SourceCred directory are not
loaded so the useful functionality is crippled.
wchargin-branch: webpack-init
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.
Currently, any non-yes keystroke causes the deply script to abort.
This is frustrating, as it may take the user several minutes of waiting
to get to the prompt, only to have the program inadvertently terminate.
As of this change, the deploys script patiently re-prompts until it gets
a valid input.
Paired with @wchargin.
Test plan: Ran locally and tested the yes, no, and invalid response
cases. Also ran the script through shellcheck.
The CNAME file is needed so that our custom domain will continue working
after deploys.
Test plan:
- Verified that the generated build now includes the cname file.
- Verified that if a CNAME file is already present, the script will
fail.
Paired with @wchargin
Summary:
Using the environment variable is the preferred way to interact with the
CLI, simply because it’s easier for users. We should demonstrate this
interface instead of the legacy flag-only version.
Paired with @decentralion.
wchargin-branch: readme-env-var
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.
This required adding a [files property] to the package.json,
otherwise oclif started complaining.
Test plan: I manually tested both CLI commands, and they seem fine.
[files property]: https://docs.npmjs.com/files/package.json#files
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.
This commit is a good faith effort to separate our dependencies (code
that SourceCred app or CLI require to run) from devDependencies (all
other deps) in our package.json.
We don't have any actual dependents, so it's hard to test this
distinction. Hence, it's a good faith effort.
Test plan:
`rm -r node_modules && yarn && yarn travis` works.
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.
Showing our example-github and example-git repos on sourcecred.github.io
is not particularly interesting. Let's show ipfs/js-ipfs instead!
Since sourcecred/sourcecred is the last repo to load, as of #531 it will
be the default option.
Test plan: Dry run of deploy script
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.
Also add config/jest/setupJest.js so we can configure jest-fetch-mock
Test plan: I have verified that mocked fetch works as expected in a
downstream commit.
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
Summary:
This provides some extra checking around `LocalStore` calls. In
particular, it fails fast on the nasty bug where storing a `Map`
actually stores the empty object (`JSON.stringify(new Map()) === "{}"`).
Similarly, retrieving a value that was stored as `undefined` will raise
an error, because `JSON.parse(JSON.stringify(undefined))` raises an
error.
This should have negligible performance impact—local storage access
should never be on a critical path. We can choose to elide this in
production if we want.
Test Plan:
Unit tests added. Manual testing of the cred explorer yields no errors.
wchargin-branch: checked-local-store
Summary:
This commit modifies components that directly depend on the
browser-specific local store implementation to instead have their
dependencies injected.
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: di-localstore
Summary:
We’d really like to be able to test components that use `LocalStore`. We
can do this by dependency-injecting the storage backend. This commit
begins that process by extracting `LocalStore` to its interface,
preserving the unique existing implementation.
wchargin-branch: extract-localstore
Summary:
This commit switches to a double-buffered PageRank implementation. When
benchmarked on `ipfs/js-ipfs`, the critical section improves from
3059 ms to 2433 ms (79.5% of original), and peak heap usage drops from
342 MB to 207 MB. (Tested non-rigorously in Chrome 67.)
Test Plan:
Existing unit tests for `sparseMarkovChainAction`,
`findStationaryDistribution`, and `pagerank` are sufficient.
wchargin-branch: pagerank-dbuf
Summary:
The PageRank functions can take a long time to compute. We’d like them
to not lock the browser, and we’d also like them to communicate with
their clients (e.g., to update a progress bar). This code updates
`findStationaryDistribution` and downstream `pagerank` to return
promises.
Test Plan:
Unit tests updated. The cred explorer (`yarn start`) still works.
Applying
```diff
diff --git a/src/core/attribution/markovChain.js b/src/core/attribution/markovChain.js
index 2acce9c..c7a7159 100644
--- a/src/core/attribution/markovChain.js
+++ b/src/core/attribution/markovChain.js
@@ -166,6 +166,7 @@ export function findStationaryDistribution(
return;
}
} while (Date.now() - start < yieldAfterMs);
+ console.log("Yielding.");
setTimeout(tick, 0);
};
tick();
```
causes the appropriate log messages to be printed in the browser—about
once every ten iterations for `sourcecred/sourcecred`.
wchargin-branch: asynchronous-pagerank
Dandelion Mané
William Chargin