fix(web-forms#864): performance for selects on large entity lists #1750
+18
−22
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| --- | ||
| "@getodk/xforms-engine": patch | ||
| "@getodk/web-forms": patch | ||
| --- | ||
|
|
||
| Fixed performance issues when rendering a select with options from a large entity list |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ import { computed, inject } from 'vue'; | |
| import MarkdownBlock from './MarkdownBlock.vue'; | ||
| import { TRANSLATE } from '@/lib/constants/injection-keys.ts'; | ||
| import type { Translate } from '@/lib/locale/useLocale.ts'; | ||
| import type { VirtualScrollerScrollIndexChangeEvent } from 'primevue'; | ||
|
|
||
| interface MultiselectDropdownProps { | ||
| readonly question: SelectNode; | ||
|
|
@@ -14,19 +15,24 @@ interface MultiselectDropdownProps { | |
| const t: Translate = inject(TRANSLATE)!; | ||
| const props = defineProps<MultiselectDropdownProps>(); | ||
|
|
||
| const INITIAL_PAGE_SIZE = 20; | ||
| const DEFAULT_PRIMEVUE_ITEM_HEIGHT = 38; | ||
|
|
||
| defineEmits(['update:modelValue', 'change']); | ||
|
|
||
| const options = computed(() => { | ||
| return props.question.currentState.valueOptions.map((option, i) => { | ||
| if (i < INITIAL_PAGE_SIZE) { | ||
| return { | ||
| value: option.value, | ||
| label: option.label.formatted, | ||
| search: option.label.asString, | ||
| loaded: true | ||
| }; | ||
| } | ||
| return { | ||
| value: option.value, | ||
| loaded: false | ||
|
Contributor
Author
There was a problem hiding this comment. This is a placeholder that's swapped out when the page is loaded.
Collaborator
There was a problem hiding this comment. The labels for options with index +20 won't be reactive anymore, right? So if a label with media takes time to load, or if a calc changes some values, is it refreshed properly? What about these scenarios but with autocomplete feature? Finding an option with index +20 and media request is slow. |
||
| }; | ||
| }); | ||
| }); | ||
|
|
@@ -35,6 +41,40 @@ const selectValues = (values: readonly string[]) => { | |
| props.question.selectValues(values); | ||
| }; | ||
|
|
||
| const handleLazyLoad = (params: VirtualScrollerScrollIndexChangeEvent) => { | ||
| const { first, last } = params; | ||
| for (let i = first; i < last; i++) { | ||
| const placeholder = options.value[i]; | ||
| if (!placeholder?.value || placeholder.loaded) { | ||
| continue; | ||
| } | ||
| const option = props.question.getValueOption(placeholder.value); | ||
| if (!option) { | ||
| // should never happen, but handle gracefully if it does | ||
| continue; | ||
| } | ||
| options.value[i] = { | ||
| value: option.value, | ||
| label: option.label.formatted, | ||
| search: option.label.asString, | ||
| loaded: true | ||
| }; | ||
| } | ||
| }; | ||
|
|
||
| const virtualScrollerOptions = computed(() => { | ||
| const isJSDOM = typeof navigator !== 'undefined' && navigator.userAgent.includes('jsdom'); | ||
| if (isJSDOM) { | ||
| return; | ||
| } | ||
| return { | ||
| lazy: true, | ||
| onLazyLoad: handleLazyLoad, | ||
| itemSize: DEFAULT_PRIMEVUE_ITEM_HEIGHT, | ||
| showLoader: true | ||
| }; | ||
| }); | ||
|
Contributor
Author
There was a problem hiding this comment. Unfortunately JSDOM completely breaks primevue's expectations, so to get the tests to pass I have to disable the lazy loading.
Collaborator
There was a problem hiding this comment. Once this is merged: #1755, this piece of code can be deleted. It's worth waiting a bit, since the other PRs are very close to merging. |
||
|
|
||
| let panelClass = 'multi-select-dropdown-panel'; | ||
| if (props.question.appearances['no-buttons']) { | ||
| panelClass += ' no-buttons'; | ||
|
|
@@ -68,6 +108,7 @@ const selectedLabels = computed(() => { | |
| option-label="search" | ||
| :panel-class="panelClass" | ||
| :model-value="question.currentState.value" | ||
| :virtual-scroller-options="virtualScrollerOptions" | ||
| @update:model-value="selectValues" | ||
| @change="$emit('change')" | ||
| > | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ import { computed, inject } from 'vue'; | |
| import MarkdownBlock from './MarkdownBlock.vue'; | ||
| import { TRANSLATE } from '@/lib/constants/injection-keys.ts'; | ||
| import type { Translate } from '@/lib/locale/useLocale.ts'; | ||
| import type { VirtualScrollerScrollIndexChangeEvent } from 'primevue'; | ||
|
|
||
| interface SearchableDropdownProps { | ||
| readonly question: SelectNode; | ||
|
|
@@ -14,31 +15,69 @@ interface SearchableDropdownProps { | |
| const t: Translate = inject(TRANSLATE)!; | ||
| const props = defineProps<SearchableDropdownProps>(); | ||
|
|
||
| const INITIAL_PAGE_SIZE = 20; | ||
| const DEFAULT_PRIMEVUE_ITEM_HEIGHT = 38; | ||
|
|
||
| defineEmits(['update:modelValue', 'change']); | ||
|
|
||
| const options = computed(() => { | ||
| return props.question.currentState.valueOptions.map((option, i) => { | ||
|
Collaborator
There was a problem hiding this comment. Is it possible to extract some of this to /lib to reuse code in both components?
Contributor
Author
There was a problem hiding this comment. I was tempted to do that, but as this is likely going to be released as a patch I didn't want to risk destabalising anything else. After realising mutliselect is deprecated and should be replaced with the select widget I think the full solution will be to merge our multiselect and searchable dropdown components into one that does both, which means extracting to lib won't be necessary any more. Issue: getodk/web-forms#865 |
||
| if (i < INITIAL_PAGE_SIZE) { | ||
| return { | ||
| value: option.value, | ||
| label: option.label.formatted, | ||
| search: option.label.asString, | ||
| loaded: true | ||
| }; | ||
| } | ||
| return { | ||
| value: option.value, | ||
| loaded: false | ||
| }; | ||
| }); | ||
| }); | ||
|
|
||
| const handleLazyLoad = (params: VirtualScrollerScrollIndexChangeEvent) => { | ||
| const { first, last } = params; | ||
| for (let i = first; i < last; i++) { | ||
| const placeholder = options.value[i]; | ||
| if (!placeholder?.value || placeholder.loaded) { | ||
| continue; | ||
| } | ||
| const option = props.question.getValueOption(placeholder.value); | ||
| if (!option) { | ||
| // should never happen, but handle gracefully if it does | ||
| continue; | ||
| } | ||
| options.value[i] = { | ||
| value: option.value, | ||
| label: option.label.formatted, | ||
| search: option.label.asString, | ||
| loaded: true | ||
| }; | ||
| } | ||
| }; | ||
|
|
||
| const virtualScrollerOptions = computed(() => { | ||
| const isJSDOM = typeof navigator !== 'undefined' && navigator.userAgent.includes('jsdom'); | ||
| if (isJSDOM) { | ||
| return; | ||
| } | ||
| return { | ||
| lazy: true, | ||
| onLazyLoad: handleLazyLoad, | ||
| itemSize: DEFAULT_PRIMEVUE_ITEM_HEIGHT, | ||
| showLoader: true | ||
| }; | ||
| }); | ||
|
|
||
| const selectedLabel = computed(() => { | ||
| const value = props.question.currentState?.value?.[0]; | ||
| if (!value) { | ||
| return []; | ||
| } | ||
| const option = props.question.getValueOption(value); | ||
| return option?.label.formatted; | ||
| }); | ||
|
|
||
| const selectValue = (value: string) => { | ||
|
|
@@ -58,6 +97,7 @@ const selectValue = (value: string) => { | |
| :options="options" | ||
| option-label="search" | ||
| option-value="value" | ||
| :virtual-scroller-options="virtualScrollerOptions" | ||
| @update:model-value="selectValue" | ||
| @change="$emit('change')" | ||
| > | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why we needed this, but it massively slowed down the rendering, because it was doing a n^2 lookup. Do we still need this validation? Is there a better way to do it than on the client every time?