Skip to content

Commit

Permalink
feat(ZNTA-975): limit to a single phrase detail request at a time
Browse files Browse the repository at this point in the history
Also only fetches phrases that do not yet have detail present.
  • Loading branch information
davidmason committed Aug 8, 2017
1 parent 5f6c8bb commit 4d8a98b
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 35 deletions.
49 changes: 22 additions & 27 deletions server/zanata-frontend/src/frontend/app/editor/selectors/index.js
Expand Up @@ -83,14 +83,31 @@ const getLocale = createSelector(
(lang, locales) => locales[lang]
)

/* Phrases and locale object, needed for phrase detail.
* Locale may be undefined
/* Visible phrases that do not have data yet.
*
* Must only use this when phrase detail is not requesting, to avoid requesting
* the same detail twice.
*
* TODO add timestamps to detail items to allow including stale phrases here
*/
export const getCurrentPagePhrasesAndLocale =
export const getMissingPhrases = createSelector(
getCurrentPagePhrases, getPhrasesDetail,
(currentPage, detail) =>
currentPage.filter(({ id }) => !detail.hasOwnProperty(id))
)

const getFetchingPhraseDetail = state => state.phrases.fetchingDetail

/* Data needed for fetching phrase detail.
*
* Phrases, locale object, and whether details is currently fetching.
* Locale may be undefined.
*/
export const getPhraseDetailFetchData =
// deep equal since filtered phrases can be a new array with the same contents
createSelectorCreator(defaultMemoize, isEqual)(
getCurrentPagePhrases, getLocale,
(phrases, locale) => ({ phrases, locale })
getMissingPhrases, getLocale, getFetchingPhraseDetail,
(phrases, locale, fetching) => ({ phrases, locale, fetching })
)

/* Detail for the current page, falling back on flyweight. */
Expand All @@ -102,28 +119,6 @@ export const getCurrentPagePhraseDetail = createSelector(
}
)

/* Visible phrases that do not have data yet.
* Careful here - do not want to keep firing requests for phrases that are
* already fetching.
*
* FIXME could include stale phrases here
* FIXME need state indicating which phrases have been requested recently
* Note: this will trigger when fetchingPhrases updates, but usually the list
* would be empty since all the ids that made it through before will be
* filtered out in the first step.
*/
// export const getMissingPhrases = createSelector(
// getCurrentPagePhrases, getPhrasesDetail,
// (currentPage, detail) => {
// // TODO add fetchingPhrases set in state
// const fetchingPhrases = new Set([])
// currentPage
// .filter(phrase => !fetchingPhrases.has(phrase.id))
// .map(({ id }) => id)
// .filter(id => !detail.hasOwnProperty(id))
// }
// )

// may be undefined
export const getLocation = state => state.routing.locationBeforeTransitions
// may be undefined
Expand Down
Expand Up @@ -8,7 +8,7 @@
*/

import watch from './watch'
import { getCurrentPagePhrasesAndLocale } from '../selectors'
import { getPhraseDetailFetchData } from '../selectors'
import { baseRestUrl } from '../api'
import { fill, isEmpty, mapValues } from 'lodash'
import { getJSON } from 'redux-api-middleware'
Expand All @@ -23,15 +23,16 @@ import {
PHRASE_DETAIL_FAILURE
} from '../actions/phrases-action-types'

// FIXME add state.phrases.fetchingDetail (bool) and only fetch when false.
// this requires watching fetchingDetail as well, which will work great to
// fetch the next batch after the current batch are merged in.
export const watchVisiblePhrasesInStore = (store) => {
const watcher = watch('phrase-detail > watchVisiblePhrasesInStore')(
() => getCurrentPagePhrasesAndLocale(store.getState()))
store.subscribe(watcher(({ phrases, locale }) => {
// optimization: only fetch if ID is not currently fetching and there is no
// recent detail in the cache.
() => getPhraseDetailFetchData(store.getState()))
store.subscribe(watcher(({ phrases, locale, fetching }) => {
// Only want one detail request at a time.
// Watcher triggers again when this flips to false.
if (fetching) {
return
}

const ids = phrases.map(phrase => phrase.id)
if (locale && !isEmpty(ids)) {
// TODO debounce to handle rapid page changes etc?
Expand Down

0 comments on commit 4d8a98b

Please sign in to comment.