Enhance Select for robust object value support (#3893)#4303
Merged
Conversation
Fix bugs preventing object values from working correctly: - valueToOption: resolve labels via labelField/valueField instead of toString() - doQueryAsync: use isEqual for dedup instead of keyBy (string-key collision) - optionRenderer: use deep equality for selected-check comparison Add lookupFn prop for async resolution of values not in the current options list, with a separate _lookupCache to avoid dropdown leakage. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Author
Contributor
Author
|
Toolbox PR: xh/toolbox#829 |
The object-handling branch was conceptually wrong — valueToOption wraps an arbitrary value INTO an option, it should not try to extract labelField/valueField from the value itself. The lookupFn prop is the proper mechanism for resolving display labels for unknown object values. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ect-support # Conflicts: # desktop/cmp/input/Select.ts # mobile/cmp/input/Select.ts
…nto enhance-select-object-support
Member
|
Could we make the |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.




Fixes bugs preventing object (non-primitive) values from working correctly, and adds a
generateOptionFnprop for resolving values not present in the current options list.Bug fixes for object values
doQueryAsync: dedupe viaisEqual/unionWithinstead ofkeyBy(string-key collision on objects).optionRenderer/ selected-option matching: use deep equality so object values match and no longer render as[object Object]or collide with one another.New
generateOptionFnprop(value) => SelectOptioncalled when a selected value is not found among the current options (e.g.queryFn-based selects, readonly forms). Returnnullto fall back to the default value-as-label behavior.findOption, so the value renders with its proper label on first paint - no async cache, reaction, or race conditions.This supersedes the async
lookupFn+_lookupCacheapproach earlier explored on this branch; the synchronous design is a much smaller, simpler footprint (just the prop plus thefindOptionfallback) and avoids the stale-cache / reaction-ordering bugs that approach was prone to.Tested on desktop and mobile via the Toolbox companion PR (employee-by-id example renders the label, not the raw id, on mount, for a value not in the options list).
Hoist P/R Checklist
Pull request authors: Review and check off the below. Items that do not apply can also be
checked off to indicate they have been considered. If unclear if a step is relevant, please leave
unchecked and note in comments.
developbranch as of last change.breaking-changelabel + CHANGELOG if so.If your change is still a WIP, please use the "Create draft pull request" option in the split
button below to indicate it is not ready yet for a final review.