-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(14815): warn when an invalid <select multiple> value is given #14816
base: main
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"svelte": patch | ||
--- | ||
|
||
Warn when an invalid `<select multiple>` value is given |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,8 @@ import { effect } from '../../../reactivity/effects.js'; | |
import { listen_to_event_and_reset_event } from './shared.js'; | ||
import { untrack } from '../../../runtime.js'; | ||
import { is } from '../../../proxy.js'; | ||
import { is_array } from '../../../../shared/utils.js'; | ||
import * as w from '../../../warnings.js'; | ||
|
||
/** | ||
* Selects the correct option(s) (depending on whether this is a multiple select) | ||
|
@@ -12,6 +14,17 @@ import { is } from '../../../proxy.js'; | |
*/ | ||
export function select_option(select, value, mounting) { | ||
if (select.multiple) { | ||
// If value is null or undefined, keep the selection as is | ||
if (value == undefined) { | ||
return; | ||
} | ||
|
||
// If not an array, warn and keep the selection as is | ||
if (!is_array(value)) { | ||
return w.select_multiple_invalid_value(); | ||
} | ||
|
||
// Otherwise, update the selection | ||
return select_options(select, value); | ||
} | ||
|
||
|
@@ -124,14 +137,12 @@ export function bind_select_value(select, get, set = get) { | |
} | ||
|
||
/** | ||
* @template V | ||
* @param {HTMLSelectElement} select | ||
* @param {V} value | ||
* @param {unknown[]} value | ||
*/ | ||
function select_options(select, value) { | ||
for (var option of select.options) { | ||
// @ts-ignore | ||
option.selected = ~value.indexOf(get_option_value(option)); | ||
option.selected = value.includes(get_option_value(option)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm pretty sure this has performance implications...I don't know why it was written this way tho There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we want better (theoretical) performance, we should use neither const values = new Set(value);
for (var option of select.options) {
option.selected = values.has(get_option_value(option));
} which is now We'll see what perf addicts say of this 👀 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be the better choice... I'm just saying that it seems an odd change I would like whoever wrote that code opinion on 😁 |
||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
import { test } from '../../test'; | ||
|
||
export default test({ | ||
warnings: [ | ||
'The `value` property of a `<select multiple>` element should be an array, but it received a non-array value. The selection will be kept as is.' | ||
] | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
<select multiple value={null}> | ||
<option>option</option> | ||
</select> | ||
<select multiple value={undefined}> | ||
<option>option</option> | ||
</select> | ||
<select multiple value={123}> | ||
<option>option</option> | ||
</select> |
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.
Why this change?
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.
It doesn't make much sense to have a templated function that does not use nor restrict its type parameter, it can be replaced with
any
. Asking forunknown[]
makes it more type-safe,// @ts-ignore
is no longer required.