g0v / vaccinate

vaxx.tw is a tool to find COVID-19 Vaccine Appointments near you
http://vaxx.tw
MIT License
33 stars 20 forks source link

Small cleanups: fix peer-dep warnings, yarn-deduplicate, use Flow's `exact_by_default`. #83

Closed chrisbobbe closed 3 years ago

chrisbobbe commented 3 years ago

Thanks for your important work on this!

My goals here are to make a few small project-level fixes, without changing user-facing behavior. Please don't hesitate to let me know if something isn't clear or seems wrong.

But in particular, after this series of commits:

I've tested this by running yarn lint and yarn flow at each commit (running yarn before those commands, on the commits that change the dependencies), and I've also tested the tip commit by running yarn frontend—though without also running yarn backend, since that seemed like a bit of work to get fully set up. I'd really appreciate if you'd run these commits (or perhaps just the tip commit) through your usual manual tests, and other automated tests I might have missed, or else give me a nudge to get yarn backend working and I'll give it a try. 🙂

When I ran yarn frontend at the tip of this series of commits, I was able to see this (and I presume I'd see more if I got set up to run yarn backend too); the loading indicator goes forever because it can't load any data:

image

The particular error message I've encountered with yarn backend is

/bin/sh: pipenv: command not found
error Command failed with exit code 127.

and presumably it wouldn't be hard for me to install something and get past this error, I just haven't yet.

kevinjcliao commented 3 years ago

Hi Chris! To get the backend to work, you should install pipenv. I'm using it as a sort of NPM for Python (per-project dependency management).

https://pipenv.pypa.io/en/latest/

Installation instructions there^^

Let me know if you have any questions, but this looks fantastic. Given you haven't touched Python, the Python checks shouldn't be necessary. I'll see if the GitHub Actions pass.

chrisbobbe commented 3 years ago

Got it, yarn backend is working now!

chrisbobbe commented 3 years ago

OK, and after running yarn backend and yarn frontend, and visiting http://localhost:5000 that was printed from yarn backend, I've poked around and things seem to be working correctly. Being new to the project, I may be less likely to spot things going wrong, but nothing stood out to me. Most of the changes deal with types and dev dependencies.

I can't say it's impossible that the yarn-deduplicate commands disrupted something, but I think it's unlikely. yarn-deduplicate has proven to be reliable, at least in the versions of it that we've used in zulip-mobile. It's been running on all commits in https://github.com/zulip/zulip-mobile since zulip/zulip-mobile@8b155e9, in 2019-11. Keep in mind, though, that not all packages follow semver (and it can get pretty close to impossible to follow perfectly), so there's always some chance that a correct deduplication does something unexpected somewhere. That's happened once in the zulip-mobile project that came to my attention; context is at https://github.com/zulip/zulip-mobile/pull/4152#issuecomment-657874893 and following, and a linked chat thread here.

I do think that the benefits of deduplicating outweigh the risks. ~All versions of a given package have some bugs, and it's easier to be acquainted with them if we don't have lots of instances of a package lying around at lots of different versions.


Observations from my manual testing:

image image image

chrisbobbe commented 3 years ago

Thanks for the review! Revision pushed.