diff --git a/src/actions/loadData.js b/src/actions/loadData.js index 5fd883da3..7f7e74081 100644 --- a/src/actions/loadData.js +++ b/src/actions/loadData.js @@ -259,41 +259,36 @@ const fetchAndCacheNarrativeDatasets = async (dispatch, blocks, query) => { secondTreeDataset: false, secondTreeName: false }; - const treeNames = _uniq(blocks.map((block) => collectDatasetFetchUrls(block.dataset)[0])); - // TODO:1050 A more performant fetching strategy would be fetching the dataset for the slide you land on, - // 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(treeNames.map((treeName) => { - // TODO:1050 - // 1. allow frequencies to be loaded for a narrative dataset here - // 2. allow loading dataset for secondTreeName - return getDataset(treeName) - .then((res) => res.json()) - .then((json) => { - jsons[treeName] = json; - if (treeName === startingTreeName) { - landingSlide.json = json; - } - return json; - }); - })).then(() => { - // TODO:1050: this dispatch is an promise side effect which means we don't guarantee - // that jsons are cached before returning below. To be able to make such a guarantee, - // there are a number of accepted ways to do async action creator functions in redux - // (https://redux.js.org/faq/actions#how-can-i-represent-side-effects-such-as-ajax-calls-why-do-we-need-things-like-action-creators-thunks-and-middleware-to-do-async-behavior) - dispatch({ - type: types.CACHE_JSONS, - jsons - }); - // We don't use the returned value of Promise.all above since we - // have already constructed the `jsons` object in order to avoid duplicate fetching. - return { - blocks, - pathnameShouldBe: startingDataset, - landingSlide - }; + const treeNames = blocks.map((block) => collectDatasetFetchUrls(block.dataset)[0]); + + // TODO:1050 + // 1. allow frequencies to be loaded for a narrative dataset here + // 2. allow loading dataset for secondTreeName + + // We block and await for the landing dataset + jsons[startingTreeName] = landingSlide.json = await getDataset(startingTreeName).then(((res) => res.json()); + + // The other datasets are fetched asynchronously + for (const treeName of treeNames) + // With this there's no need for Set above + jsons[treeName] = jsons[treeName] || getDataset(treeName).then((res) => res.json()); + + // I don't think the below here is a real problem for any practical case (?) + + // TODO:1050: this dispatch is an promise side effect which means we don't guarantee + // that jsons are cached before returning below. To be able to make such a guarantee, + // there are a number of accepted ways to do async action creator functions in redux + // (https://redux.js.org/faq/actions#how-can-i-represent-side-effects-such-as-ajax-calls-why-do-we-need-things-like-action-creators-thunks-and-middleware-to-do-async-behavior) + dispatch({ + type: types.CACHE_JSONS, + jsons }); + + return { + blocks, + pathnameShouldBe: startingDataset, + landingSlide + }; }; diff --git a/src/actions/navigation.js b/src/actions/navigation.js index 100ad3344..cad4e925b 100644 --- a/src/actions/navigation.js +++ b/src/actions/navigation.js @@ -25,27 +25,19 @@ export const chooseDisplayComponentFromURL = (url) => { return "datasetLoader"; // fallthrough }; -/* Try to get a JSON from the redux state where it may already have - * been cached. If it's not there, we should fetch it. The latter part - * has yet to be implemented since the only instance where we cache JSONs - * at this point fetches all needed JSONs by a given narrative before rendering. - * In the future, when we allow for more performative loading of JSONs in the backgroud, - * we won't be able to predict their existence in the cache and so will need to fetch - * here upon a "cache miss" +/* + * All the Fetch Promises are created before first render. When trying the cache we `await`. + * If the Fetch is not finished, this will wait for it to end. Subsequent awaits will immeditaly return the result. + * For the landing dataset, no problem either because await on a value just returns the value. */ const tryCacheThenFetch = async (mainTreeName, secondTreeName, state) => { if (state.jsonCache && state.jsonCache.jsons && state.jsonCache.jsons[mainTreeName] !== undefined) { return { - json: state.jsonCache.jsons[mainTreeName], - secondJson: state.jsonCache.jsons[secondTreeName] + json: await state.jsonCache.jsons[mainTreeName], + secondJson: await state.jsonCache.jsons[secondTreeName] }; } - // TODO:1050 we should fetch a dataset here if it is not in the cache - // instead of throwing an error. As the error message suggests, the reason - // we throw is because our current fetching strategy should prevent a cache miss. - // We will want to implement a real fetch here when we move to a more performant - // fetching strategy (see actions/loadData.js) - throw new Error("This should not happen given that we naively pre-fetch all datasets for any narrative before rendering"); + throw new Error("This should not happen given that we start fetching all datasets before rendering"); }; /* changes the state of the page and (perhaps) the dataset displayed.