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
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
William Chargin
Dandelion Mané
Claire L