Open Sourcing checklist

[mdirolf]

I've invited a couple of folks as contributors to look over the codebase and get it ready for open sourcing. Let's use this discussion to coordinate

I'm not able to add fine-grained permissions for you so please refrain from pushing to master. If you have changes to make please use your own branch and make a PR when done - this will be more straightforward once the repo is public and folks can fork it rather than needing to be added as contributors.

I'd like to get it so that a new contributor can follow steps in the README in order to run the test suite and run the app locally. I'll try to add some basics to the README today but it would be awesome if you folks could try both of those things, ask questions where you get stuck, record any missing steps, etc.

Please use this discussion to note anything else you think we should do before making the source code public! I have done a rough audit for secrets and not found anything, but if you notice anything that you don't think ought to be public definitely speak up!

Thank you!

[mdirolf]

I've gone through and set up a local version of the project from scratch today, and tried to document as I went along in the README. Please give that a shot and feel free to edit/expand the readme (or ask any questions!) anywhere you think I'm missing some details.

It's a bit trickier to run locally than I'd imagined, as you have to create your own firebase project to connect to. Perhaps we could try to use the firebase emulator suite to bootstrap this for people running against a local version of firebase services instead of a live version?

[mdirolf]

Some more updates to the README - notably instructions for building the cluedb and building/uploading the wordlist to cloud storage. Hopefully these instructions are follow-able but it is a bit complicated for somebody trying to get started. I created this issue to try to remove that step: #249

That being said - I was able to get the constructor working locally using the instructions that are now in the README - hopefully somebody else manages to confirm that as well!

[mdirolf]

Lastly, I added instructions for running the test suite. This should be very easy (famous last words) once you've managed to set up the local dev environment.

[jmviz]

I've followed the steps relatively without issue up to:

$ GOOGLE_APPLICATION_CREDENTIALS=../serviceAccountKey.json ./scripts/bootstrapDatabase.ts

where I get an error: Unexpected token ' in JSON at position 0. Any idea what might be the issue there?

[mdirolf]

That is awesome news - thanks @jmviz!

I'm so ashamed to admit it but that test has actually been broken for a little while. The functionality it's supposed to be testing is working as intended and I could not figure out why it wasn't passing, so I decided to come back to it. It's probably time to actually do so!

[mdirolf]

Other than the issue you mentioned w/ bootstrapDatabase.ts was there anything you thought could be made clearer in the instructions or any other areas for improvement? I do hope the whole process can be streamlined with #249, #250, and #251

[jmviz]

I did have to unquote the json args in the other scripts as you mentioned. So finding a way to make those scripts cross-platform would be nice. Also, before running npx firebase use --add, I had to export PATH="$(yarn bin):$PATH" to find firebase and then firebase login. When running npx firebase deploy --only firestore, it took about 20 minutes for firebase to build everything; before that was done, doing yarn dev and opening http://localhost:3000/ resulted in errors.

Those issues you link I think would definitely make it easier / more straightforward. For the wordlist in particular it seems to me it would make sense for it to already be compiled and ready to go, perhaps as part of the repo itself, or accessed from crosshare.org as #249 suggests.

[mdirolf]

Oo good to know about needing to add firebase to the path and login! Also you're right I should make a note about the index build time - I'm surprised it took so long but it definitely needs to finish.

Agreed that the wordlist feels like the lowest hanging fruit to improve. I am curious to spend some time with the emulators too and see if that could make things substantially simpler.

[mdirolf]

I believe this commit should fix the scripts issue - moved the tsconfig into a separate file in the scripts directory and removed those arguments altogether: afdccbb

[TechnoCrepe]

I'm running on Windows, and I too roughly got to the same spot as @jmviz with running GOOGLE_APPLICATION_CREDENTIALS=<stuff>

At a glance, I think it would be possible to get it running locally on Windows, but it may require some alternatives (e.g. bootstrapDatabase.ts is currently treated like a bash script).

Before I forget, 2 things I did do while setting up was ...

• Install nvm for windows and install node 12 via that.
• I also needed to run npx firebase login prior to npx firebase use --add

It's currently 1 A.M. and I'm starting to re-read sentences a few times over, so I thought I'd jot this down and continue getting it to work tomorrow.

[jmviz]

I was able to get it working on my Windows machine using Windows Subsystem for Linux, which I would recommend if it looks like a pure Windows setup will be a headache (I didn't try).

[mdirolf]

Awesome thanks @jmviz - I'm going to add that to the README

[TechnoCrepe]

Yeah, I debated WSL at first, then tried the pure-Windows route for a brief moment, before realizing it would take way too much effort 😛
But I was able to get WSL running the other night and got the local server going too

@mdirolf - I actually got a handful of test failures....

Something like...

Snapshot Summary
 › 3 snapshots failed from 2 test suites. Inspect your code changes or press `u` to update them.

Test Suites: 3 failed, 21 passed, 24 total
Tests:       4 failed, 197 passed, 201 total
Snapshots:   3 failed, 157 passed, 160 total
Time:        57.196 s
Ran all test suites.

I don't suppose both of you are on the East Coast?
If so there may be a bug with the snapshots regarding timezones, as I am on the West Coast, for example....

[TechnoCrepe]

It seems there is an open issue on date-fns/date-fns#2151, which is what is used to generate these strings.
There are suggestions around (like this one date-fns/date-fns#2366 (comment)) that suggest to just use Date's .toISOString() instead.

I'll try and fix all the snapshots that are have this since it's affecting me and send out a PR.

[mdirolf]

Great thanks @TechnoCrepe - yup I'm on east coast and haven't run them anywhere else so that seems likely to be the issue!

Semi-relatedly - the tests are definitely the portion of the codebase I'm least satisfied with. They're not nearly exhaustive, they are a bit flaky, and they're slow to boot. So any improvements there are greatly appreciated!

[mdirolf]

There are a couple of open issues that have come up following this discussion (#249, #250, #251), but since several of you have gotten things running and there haven't been any show-stopping discoveries about the code base, I'm leaning towards flipping the switch on opening the codebase soon. Anybody have any reasons we should wait?

[jmviz]

I say go for it :), more eyes will result in more improvements more quickly!

One small thing I noticed is there is a ToS page, bits of which could be updated to reflect the new open-source license. But it doesn't look like that page is accessible from any other page at the moment, so it is maybe a nonissue.

[TechnoCrepe]

Yeah I think it'll be fine. I think the "good first issue" tag is also a good idea to help folks get started with some direction.

[mdirolf]

OK thanks guys! I have flipped the switch so the repo should be public now 🥳. As such I've removed you folks as collaborators so that we can try what I think ought to be the more "normal" workflow - i.e. fork/PR for changes rather than pushing to this repo directly. I think I have migrated any action items from this discussion into normal issues But feel free to create any more if there's something I missed. Thanks again!