fix(web-forms#864): performance for selects on large entity lists - #1750
fix(web-forms#864): performance for selects on large entity lists#1750garethbowen wants to merge 6 commits into
Conversation
🦋 Changeset detectedLatest commit: 922bc97 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| value: option.value, | ||
| label: option.label.formatted, | ||
| search: option.label.asString, | ||
| loaded: false |
There was a problem hiding this comment.
This is a placeholder that's swapped out when the page is loaded.
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.
| itemSize: DEFAULT_PRIMEVUE_ITEM_HEIGHT, | ||
| showLoader: true | ||
| }; | ||
| }); |
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.
| return item.value === value; | ||
| }); | ||
|
|
||
| const valueOption = this.mapOptionsByValue().get(value); |
There was a problem hiding this comment.
This seems to work, and is far faster than scanning through each of the valueOptions. The comment above is concerning though and makes me think I might have missed something.
| return props.question.currentState.valueOptions.map((option) => { | ||
| const label = props.question.getValueOption(option.value); | ||
| if (label == null) { | ||
| throw new Error(`Failed to find option for value: ${option.value}`); |
There was a problem hiding this comment.
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?
|
NB: multiselect is deprecated, and should be replaced with select. I've put that change off because it would be too disruptive at this point. |
latin-panda
left a comment
latin-panda
left a comment
There was a problem hiding this comment.
I haven't had a chance to set up my environment for testing this yet. The code looks good so far—just a few questions below while I get my environment ready.
| value: option.value, | ||
| label: option.label.formatted, | ||
| search: option.label.asString, | ||
| loaded: false |
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.
| const label = props.question.getValueOption(option.value); | ||
| if (label == null) { | ||
| throw new Error(`Failed to find option for value: ${option.value}`); | ||
| return props.question.currentState.valueOptions.map((option, i) => { |
There was a problem hiding this comment.
Is it possible to extract some of this to /lib to reuse code in both components?
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
In actual fact my changes just broke autocomplete altogether - anything that wasn't "loaded" was never returned. So I added back the Unfortunately there was another problem which is that the Using the virtual scroller without lazy is still a big improvement so that's probably enough for this patch.
I've tested reactivity with the above changes and everything I tried works. I don't think it's possible to have media in a dropdown label is it? The code just calls the markdown component without the media component. NB: This change only impacts dropdowns, not radios, checkboxes, etc. |
|
The failing e2e test is also failing on So I think it can be ignored here... |
2 participants
Closes getodk/web-forms#864
What has been done to verify that this works as intended?
Why is this the best possible solution? Were any other approaches considered?
How does this change impact users? Describe intentional behavior changes from code updates. What are the regression risks?
There's a slight regression where short select lists are now shown with whitespace at the bottom. I think it's unavoidable when lazy loading.
Does this change require updates to user documentation? If so, please file an issue here and include the link below.