Add timeout queue #1183
Add timeout queue #1183
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
also rework narrative json fetching code, fixes bug with: "explore the data yourself" and "return to narrative" buttons
… file This is in preparation for client-side parsing of narrative files. A optional `type` URL query allows the same API to return the markdown file (see the documentation changes in this commit for more details). There are other possibilities, such as using the content-type field in the header, but I felt this was the clearest.
Here we add the framework to (eventually) parse the response from `getNarrative` as a markdown file and interpret this client-side. To facilitate backward compatibility in the cases where a server does not respect the `?type=md` query and continues to return a JSON, if the file doesn't look like markdown we will attempt to parse it as JSON. To mimic such a server, we explicitly request "type=json" in this commit, as the code to actually do the client-side md-parsing is not yet implemented.
This commit shifts the parsing of the markdown file (md + yaml frontmatter) into a centralised function which is imported by both the server and the client, allowing us to maintain backward compatible server-side parsing as well as client-side parsing, whilst avoiding code duplication and the inevitable divergence of functionality. Our approach to conversion of YAML frontmatter -> markdown is essentially unchanged. The algorithm for parsing of the main body has been rewritten to be more robust. Unit tests are added to cover the various ways we can define authors, links & affiliations in the YAML frontmatter of narratives.
This commit was prompted by a bug resulting from the URL query representing two different concepts: what is displayed in the URL and the app state that this drives.
Previously we approached this with two arguments to the state-setting-function: `query` and `queryToDisplay`. Here we remove the latter and interpret the former as synonymous with the URL. This results in simplified logic where the logic for interpreting what app state a narrative page desires (e.g. `query={n: 3}` -> `{c: "region", ...}) now resides within the `createStateFromQueryOrJSONs` function.
This commit represents a simplification of the page-change logic when navigating between pages (the `EXPERIMENTAL_showMainDisplayMarkdown` action is unnecessary as the `changePage` action will correctly set the panels). Additionally we rename the react component to remove the "experimental" prefix as this has been in widespread use for a number of months now.
This commit represents a big simplification of how we toggle into and out of narrative mode. This happens in two places: the "exit narrative mode" button on the last narrative slide and the "explore the data" button (desktop only). The desired behavior is that we switch to "interactive mode" (i.e. with the controls sidebar) but with a "return to narrative" button (desktop only). The URL should change from a narrative pathname and n=<slide_number> query to the pathname of the dataset & query describing the view settings.
The code is simplified to use a consistent action for all the above behavior ("TOGGLE_NARRATIVE"). This allows multiple datasets to work appropriately, as well as window resizing triggering a switch to mobile display (or vice versa).
This resolves a long-standing development paper-cut where the narrative URL would disappear when hot-reloading (due to the unmounting behavior).
Co-authored-by: eharkins <eli.harkins@gmail.com>
Narrative mode improvements
Improve redux state recalc on narrative slide changes. Closes #1191
Fix the shift/option + mouseWheel zoom bug in entropy panel
Currently there is no vertical space between paragraphs. This fix is for desktop only as padding was already in place on mobile. Closes #1196
The narrative to test missing dataset was confusing as the dataset (a "nextstrain community" URL) was invalid for a local auspice server but valid for the nextstrain.org server. Making it a clearly non-existing dataset URL makes it less confusing when testing functionality within nextstrain.org.
src/components/map/map.js
Outdated
| @@ -640,10 +643,12 @@ class Map extends React.Component { | |||
| const maxZoom = this.getMaxZoomForFittingMapToData(); | |||
| // first, clear any existing timeout | |||
| if (this.bounds_timeout) { | |||
| window.clearTimeout(this.bounds_timeout); | |||
| removeTimeout(this.bounds_timeout); | |||
There was a problem hiding this comment.
needs to be removeTimeout('map', this.map_timeout)
src/components/tree/tangle/index.js
Outdated
| @@ -72,11 +73,11 @@ class Tangle extends React.Component { | |||
| componentDidUpdate(prevProps) { | |||
| if (this.props.width !== prevProps.width || this.props.height !== prevProps.height) { | |||
| if (this.timeout) { | |||
| window.clearTimeout(this.timeout); | |||
| removeTimeout(this.timeout); | |||
There was a problem hiding this comment.
needs to be removeTimeout('tree', this.timeout)
| }; | ||
|
|
||
| export const removeTimeout = (component, timeout) => { | ||
| if (timeouts[component].some((to) => to === timeout)) { |
There was a problem hiding this comment.
Consider adding a safeguard here such as
if (!timeouts[component]) {
console.error("Cannot remove timeout for component", component, "ref", timeout);
return;
}or wrapping with try/catch
There was a problem hiding this comment.
Abstracting timeouts into a helper function is great. I had to make a couple of edits to avoid it crashing in narratives mode (see in-line comments) which I think were just typos.
Prior to this PR we did things such as keeping a reference to the last setTimeout call and clearing it if it existed the next time round (e.g. this.map_timeout). With the addition of a utility to do this I think storing these is unnecessary, and I think it'd be nicer (and simpler) to rely on the utility to handle this -- for instance giving each timeout a name and let the util clear any that haven't yet fired. E.g. trigger a timeout via addTimeout('fitMapBounds', () => {...}, t, args and let the utility clear any yet-to-be-run timeouts for the signature 'fitMapBounds'. This will improve the readability of the code and be one less thing to worry about when developing.
Related to this, I'm almost certain that for a every setTimeout call in Auspice (e.g. "delay to change map bounds") we always want to clear any yet-to-run timeouts (of the same "type") if we call it again! There have been a few nasty bugs related to this over the years. This is why I think it'd be great for the utility to handle it, so that you could be confident that calling addTimeout will cancel out any yet-to-fire ones.
Perhaps we also pass the component name to addTimeout in addition, as I really like the simplicity of this:
componentWillUnmount() {
clearAllTimeouts('map');
}Thanks!
|
Something gone terribly wrong when I merged upstream changes to my fork and rebase the feature branch... Gonna redo the pull request again... |
Description of proposed changes
Replace anonymous setTimeout in components with mapped timeout arrays that's traceable and clearable for house keeping at unmount.
Related issue(s)
Fixes #1115
Testing
Tested by navigating around datasets and new unit test for the util/timeoutqueue added.
Thank you for contributing to Nextstrain!