From 0733b47827fb9d2cbcc7ee20b76862f7a1335030 Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Thu, 29 Aug 2024 17:54:41 -0700 Subject: [PATCH] [tech debt] Convert Promise syntax to modern async/await - remove separate async `resolveOptionsLoader` fn - for some reason this separate method causes the EuiSelectableList to remount completely, no idea why :T + add missing clearTimeout for scheduled cache wipe, which could potentially cause stale errors after unmount + add missing test case for `cache` config --- .../field_value_selection_filter.spec.tsx | 62 +++++++++ .../filters/field_value_selection_filter.tsx | 131 ++++++++---------- 2 files changed, 123 insertions(+), 70 deletions(-) diff --git a/packages/eui/src/components/search_bar/filters/field_value_selection_filter.spec.tsx b/packages/eui/src/components/search_bar/filters/field_value_selection_filter.spec.tsx index 962322d4e12..ee42c9ef8fc 100644 --- a/packages/eui/src/components/search_bar/filters/field_value_selection_filter.spec.tsx +++ b/packages/eui/src/components/search_bar/filters/field_value_selection_filter.spec.tsx @@ -502,4 +502,66 @@ describe('FieldValueSelectionFilter', () => { .eq(0) .should('have.attr', 'title', 'Bug'); }); + + it('caches options if options is a function and config.cache is set', () => { + // Note: cy.clock()/cy.tick() doesn't currently work in Cypress component testing :T + // We should use that instead of cy.wait once https://github.com/cypress-io/cypress/issues/28846 is fixed + const props: FieldValueSelectionFilterProps = { + index: 0, + onChange: () => {}, + query: Query.parse(''), + config: { + type: 'field_value_selection', + field: 'tag', + name: 'Tag', + cache: 5000, // Cache the loaded tags for 5 seconds + options: () => + new Promise((resolve) => { + setTimeout(() => { + resolve(staticOptions); + }, 1000); // Spoof 1 second load time + }), + }, + }; + cy.spy(props.config, 'options'); + + const reducedTimeout = { timeout: 10 }; + const assertIsLoading = (expected?: Function) => { + cy.get('.euiSelectableListItem', reducedTimeout).should('have.length', 0); + cy.get('[data-test-subj="euiSelectableMessage"]', reducedTimeout) + .should('have.text', 'Loading options') + .then(() => { + expected?.(); + }); + }; + const assertIsLoaded = (expected?: Function) => { + cy.get('.euiSelectableListItem', reducedTimeout).should('have.length', 3); + cy.get('[data-test-subj="euiSelectableMessage"]', reducedTimeout) + .should('not.exist') + .then(() => { + expected?.(); + }); + }; + + cy.mount(); + cy.get('button').click(); + assertIsLoading(); + + // Wait out the async options loader + cy.wait(1000); + assertIsLoaded(() => expect(props.config.options).to.be.calledOnce); + + // Close and re-open the popover + cy.get('button').click(); + cy.get('button').click(); + + // Cached options should immediately repopulate + assertIsLoaded(() => expect(props.config.options).to.be.calledOnce); + + // Wait out the remainder of the cache, loading state should initiate again + cy.get('button').click(); + cy.wait(5000); + cy.get('button').click(); + assertIsLoading(() => expect(props.config.options).to.be.calledTwice); + }); }); diff --git a/packages/eui/src/components/search_bar/filters/field_value_selection_filter.tsx b/packages/eui/src/components/search_bar/filters/field_value_selection_filter.tsx index 40c1c259f91..da21b2041ce 100644 --- a/packages/eui/src/components/search_bar/filters/field_value_selection_filter.tsx +++ b/packages/eui/src/components/search_bar/filters/field_value_selection_filter.tsx @@ -87,6 +87,8 @@ export class FieldValueSelectionFilter extends Component< FieldValueSelectionFilterProps, State > { + cacheTimeout: ReturnType | undefined; + constructor(props: FieldValueSelectionFilterProps) { super(props); const { options } = props.config; @@ -125,83 +127,68 @@ export class FieldValueSelectionFilter extends Component< }); } - loadOptions() { - const loader = this.resolveOptionsLoader(); + loadOptions = async () => { + let loadedOptions: FieldValueOptionType[]; this.setState({ options: null, error: null }); - loader() - .then((options) => { - const items: { - on: FieldValueOptionType[]; - off: FieldValueOptionType[]; - rest: FieldValueOptionType[]; - } = { - on: [], - off: [], - rest: [], - }; - - const { query, config } = this.props; - - if (options) { - options.forEach((op) => { - const optionField = op.field || config.field; - if (optionField) { - const clause = - this.multiSelect === 'or' - ? query.getOrFieldClause(optionField, op.value) - : query.getSimpleFieldClause(optionField, op.value); - const checked = this.resolveChecked(clause); - if (!checked) { - items.rest.push(op); - } else if (checked === 'on') { - items.on.push(op); - } else { - items.off.push(op); - } - } - return; - }); - } - this.setState({ - error: null, - activeItemsCount: items.on.length, - options: { - unsorted: options, - sorted: [...items.on, ...items.off, ...items.rest], - }, - }); - }) - .catch(() => { - this.setState({ options: null, error: 'Could not load options' }); - }); - } - - resolveOptionsLoader: () => OptionsLoader = () => { - const options = this.props.config.options; - if (isArray(options)) { - return () => Promise.resolve(options); + const { options, cache } = this.props.config; + try { + if (isArray(options)) { + // Synchronous options, already loaded + loadedOptions = options; + } else { + // Async options loader fn, potentially with a cache + loadedOptions = this.state.cachedOptions ?? (await options()); + + // If a cache time is set, populate the cache and schedule a cache reset + if (cache != null && cache > 0) { + this.setState({ cachedOptions: loadedOptions }); + this.cacheTimeout = setTimeout(() => { + this.setState({ cachedOptions: null }); + }, cache); + } + } + } catch { + return this.setState({ options: null, error: 'Could not load options' }); } - return () => { - const cachedOptions = this.state.cachedOptions; - if (cachedOptions) { - return Promise.resolve(cachedOptions); - } + const items: Record = { + on: [], + off: [], + rest: [], + }; - return (options as OptionsLoader)().then((opts) => { - // If a cache time is set, populate the cache and also schedule a - // cache reset. - if (this.props.config.cache != null && this.props.config.cache > 0) { - this.setState({ cachedOptions: opts }); - setTimeout(() => { - this.setState({ cachedOptions: null }); - }, this.props.config.cache); - } + const { query, config } = this.props; - return opts; + if (loadedOptions) { + loadedOptions.forEach((op) => { + const optionField = op.field || config.field; + if (optionField) { + const clause = + this.multiSelect === 'or' + ? query.getOrFieldClause(optionField, op.value) + : query.getSimpleFieldClause(optionField, op.value); + const checked = this.resolveChecked(clause); + if (!checked) { + items.rest.push(op); + } else if (checked === 'on') { + items.on.push(op); + } else { + items.off.push(op); + } + } + return; }); - }; + } + + this.setState({ + error: null, + activeItemsCount: items.on.length, + options: { + unsorted: loadedOptions, + sorted: [...items.on, ...items.off, ...items.rest], + }, + }); }; resolveOptionName(option: FieldValueOptionType) { @@ -264,6 +251,10 @@ export class FieldValueSelectionFilter extends Component< if (this.props.query !== prevProps.query) this.loadOptions(); } + componentWillUnmount() { + clearTimeout(this.cacheTimeout); + } + render() { const { query, config } = this.props;