Create reactive datastore for models with Mobx

[macfarlandian]

Description of the change

This adds reactivity to our data models with MobX and organizes them into a central data store. This was mostly pretty straightforward, since this application doesn't do much yet, but there was some weird stuff that seemed worth highlighting as we figure out how best to use Mobx.

• a lot of the data in our models is essentially static. I didn't make any of those properties observable because they should never change; for extra clarity I also marked them as readonly to the extent possible. The notable exception is in the relationship between Metrics and Collections; because it's cyclic, one side has to get updated after instantiation, so it can't be readonly. It is still not observable, though, and I left a comment to that effect for posterity.
• the application's "current" tenant (i.e. the one whose page you are on) is now a piece of observable data. Maybe there will be more properties like this once the actual site navigation is in place (the current Metric, the current Narrative, etc), but I didn't want to go overboard with that prematurely.
• the existing data model tests were still passing after I added all the reactivity features, so technically I didn't really need to update them. However, once I turned on the Mobx runtime "linting" features (which they recommend for newbies) the console started yelling at me, and it seemed like good practice anyway to have the tests be reactive when the UI components would have to be as well. I think the only really contrived part is the async data fetch test; it's not doing anything bad, necessarily, but I don't expect that the UI will have to observe the fetch Promise the way I do here (it will just observe the loading and records properties).

(Note that this is stacked on top of #273 so you may want to look at that one first if you haven't yet and it's still open)

Type of change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Configuration change (adjusts configuration to achieve some end related to functionality, development, performance, or security)

Related issues

Closes #262

Checklists

Development

These boxes should be checked by the submitter prior to merging:

• Manual testing against realistic data has been performed locally

Code review

These boxes should be checked by reviewers prior to merging:

• This pull request has a descriptive title and information useful to a reviewer
• This pull request has been moved out of a Draft state, has no "Work In Progress" label, and has assigned reviewers
• Potential security implications or infrastructural changes have been considered, if relevant

[coveralls]

Pull Request Test Coverage Report for Build 422168898

• 11 of 12 (91.67%) changed or added relevant lines in 4 files are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage increased (+0.07%) to 60.887%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
spotlight-client/src/index.tsx	0	1	0.0%

Files with Coverage Reduction	New Missed Lines	%
spotlight-client/src/index.tsx	1	0%

Totals
Change from base Build 422156907:	0.07%
Covered Lines:	1166
Relevant Lines:	1868

---

💛 - Coveralls

[jovergaag]

This is cool. It took me forever to review because I had to go back and re-read the MobX docs, but am understanding things much better now.

I just have a couple of comments about if and how the metric data will eventually be added to the DataStore. Not suggesting at all that changes need to be made, just curious about design decisions.

[jovergaag]

I'm trying to wrap my head around when it would make sense to make a store vs making observable attributes on a model like you've done here.

It makes sense that you would want isLoading to be observable so that components can change what they render based on this status boolean. Why did you choose to just set this in an action here vs creating a store and setting this in the store like you did for the TenantStore.currentTenant?

[jovergaag]

Is your intention to eventually store the metric data as part of the TenantStore using getMetricsForTenant?

[macfarlandian]

I did it this way because the relationship between tenant and metric is strictly hierarchical ... the Mobx docs advise against normalizing those kinds of relationships or making your stores resemble relational database tables or something like that. Instead the metrics are literally nested within TenantStore.currentTenant.

As to your second question, it's a good question ... I don't really know the answer yet. There are some places where a currentMetric would make sense (e.g. you have navigated to the page for a single metric), but in other places you will see multiple metrics at once. There is nothing nested below a Metric so it wouldn't serve the same limiting function to designate one as "current" and thus it might not even be necessary to do it at all? Figuring this out will be a big part of #274.

[jovergaag]

Great tests. Smart to use when reaction for these tests!

[jovergaag]

Very nice. 🐶