Enable a narrative to source multiple builds #1164
Conversation
using redux to cache JSONs
this completes a bunch of TODOs (search TODO:1071 for remaining items): 1. clear the json cache when unmounting narrative component 2. rerender mounted react components with new dataset upon redux state update this is achieved using react "key" props (https://reactjs.org/docs/lists-and-keys.html#keys) 3. wait for all datasets to be fetched using promise.all 4. fix loading narratives from not the first page 5. use the appropriate dataset when leaving the narrative page to explore a dataset
There was a problem hiding this comment.
Thanks @eharkins (and @salvatore-fxpig). This is looking great. I'll think a bit about how we're calling changePage over the coming days to make sure this is the right direction.
In addition to the in-line comments, In terms of to-dos before merge / auspice release we'll need:
- A more expansive narrative to test with (e.g. using entropy panel, multiple datasets). This doesn't have to be huge for the purposes of this PR.
- Write a small script to check all files in github.com/nextstrain/narratives and ensure each uses the same dataset for each block. We can fix these as we find them.
-
Write a test narrative with a misformed / non-existant dataset in one of it's blocks. I imagine there are narratives out there with this.Experiment with one of the incorrectly formatted narratives @eharkins identified above. This would have rendered fine on current master, but on this PR would result in the rejection of thePromise.allwe use to fetch datasets. Ideally, we'd use the previous "good" dataset, but at the least we can print a helpful error notification.
src/actions/loadData.js
Outdated
| // then fetching all the rest in the background so we can use them from the cache upon changing slides. | ||
| // Doing that presents the risk of a race case (if you change pages faster than a dataset can be fetched) so we are avoiding it for now. | ||
| // Instead we use Promise.all to ensure all the datasets are fetched before we render. | ||
| return Promise.all(blocks.map((block, i) => { |
There was a problem hiding this comment.
This actually makes a request for every block, regardless of whether the blocks use the same dataset (treeName) -- i.e. the test narrative fetches the zika dataset twice and the mumps/na dataset twice. Prior to the Promise.all I think we want to make a set of all treeName and secondTreeName across the narrative blocks, then perform a Promise.all on that (unique) set.
There was a problem hiding this comment.
Added a commit that should fix this. Thanks for pointing it out!
src/actions/loadData.js
Outdated
| secondTreeDataset: false, | ||
| secondTreeName: false | ||
| }; | ||
| // TODO:1050 A more performant fetching strategy would be fetching the dataset for the slide you land on, |
There was a problem hiding this comment.
Agree that this will add complexity and is not strictly necessary, thus it should be addressed as a subsequent PR. The current implementation fetches the datasets in parallel (via Promise.all) which it would be interesting to time against a fetch of a single dataset.
| jsons: {} /* object with dataset names as keys and loaded dataset / narrative jsons as values */ | ||
| }, action) => { | ||
| switch (action.type) { | ||
| case types.CACHE_JSONS: |
There was a problem hiding this comment.
Since we have the ability to return to the current narrative -- click "explore the dataset" then click "return to narrative" -- we want to preserve the cache until a new dataset is loaded*, i.e. the CLEAN_START action. Currently this series of events causes Auspice to crash. This will also remove the need for a separate action here when the narratives unmount. (I realise I may have suggested to clear out the cache when we click "explore the dataset"!!)
* This should have the effect that changing the dataset to on which happened to be cached will load it from the cache rather than fetching it.
There was a problem hiding this comment.
Makes sense! Just pushed a fix that seems to fix the bug with this behavior and only clear the cache on CLEAN_START
| })); | ||
|
|
||
| }; | ||
| if (this.props.blocks[idx].dataset !== this.props.blocks[this.props.currentInFocusBlockIdx].dataset) { |
There was a problem hiding this comment.
Is this the only section which needs to be transferred to the mobile narrative display for that to work?
|
Uuuh I was waiting for this to come up again! PS: I think adding uniq from lodash just changed the other-vendors chunkhash, if you're already publishing somewhere with long-term caching I would suggest using |
|
I think the async stuff can be baked in pretty easily, something along these lines should do the trick |
|
Dumping some stuff here: #1167, hopefully makes it easier to play with
|
also rework narrative json fetching code, fixes bug with: "explore the data yourself" and "return to narrative" buttons
|
Thanks @salvatore-fxpig !
Makes sense. The reason I used _uniq instead of Sets was only because I wanted to use .map() on the unique list of datasets and not have to convert the Set back to an array to do that. And because when I used the jsons object keys to ensure no duplicate fetches, it didn't have that behavior because of a misunderstanding of promises on my part. I'm sure there is a better way though so I will check out 1167 and give it a test! |
The original design of nextstrain narratives was that each "section" (paragraph) should define its own dataset, such that a single narrative could display different datasets (or the same one if each section specified the same dataset). This was never implemented -- i.e. in auspice currently only the dataset specified in the YAML frontmatter is ever used -- which has resulted in the inadvertent creation of narratives which specify different (and perhaps invalid) datasets. Nearly every narrative file in this repo had inadvertently defined multiple datasets! Often this appears to be due to copy&paste of paragraphs from previous narratives etc. Auspice will soon "correctly" interpret these multiple datasets (nextstrain/auspice#1164). This commit changes 206 narratives which previously specified multiple datasets so that each one now only specifies the dataset defined in the YAML frontmatter. This will preserve the current behavior in future versions of Auspice. The only exception is the "test_multiple-datasets.md" narrative. There is also a small script which can be run periodically to identify narratives which define multiple narratives. Closes #8
Description of proposed changes
This builds on an existing contribution #1071 (thanks to the amazing work of @salvatore-fxpig
!) to address issue #1050, i.e. enabling a narrative to source multiple builds (datasets). It mostly works, but needs more testing.
Some issues which might prevent this from getting merged (and I am still working to reproduce and define so that we can address them):
Some goals it has inspired:
Some TODO items that can become separate issues (i.e. don't prevent this from being merged if we don't mind these imperfections), which we will still appreciate any help with cc @salvatore-fxpig:
Testing
I have been testing with this example narrative created by @jameshadfield and extending it as necessary. As James pointed out, maintaining a huge narrative (or set of narratives) that runs through many combinations of dataset and queries (and thus app states) could be immensely helpful in testing narratives manually as well as with automated tests. I will create an issue as a stub for this and as a place for discussion of how we want to use that big test narrative.
Thank you for contributing to Nextstrain!