8000 fix(14815): warn when an invalid <select multiple> value is given by GauBen · Pull Request #14816 · sveltejs/svelte · GitHub
[go: up one dir, main page]
Skip to content

fix(14815): warn when an invalid <select multiple> value is given #14816

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
May 20, 2025
Merged

fix(14815): warn when an invalid <select multiple> value is given #14816

merged 4 commits into from
May 20, 2025

Conversation

GauBen
Copy link
Contributor
@GauBen GauBen commented Dec 23, 2024 •
edited
Loading

Before submitting the PR, please make sure you do the following

Emit a warning instead of crashing the runtime when an invalid value is provided

Closes #14815

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with feat:, fix:, chore:, or docs:.
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.
  • If this PR changes code within packages/svelte/src, add a changeset (npx changeset).

Tests and linting

  • Run the tests with pnpm test and lint the project with pnpm lint
8000

@changeset-bot changeset-bot
Copy link
changeset-bot bot commented Dec 23, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: 2370dac

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
svelte Patch

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

@Rich-Harris
Copy link
Member

preview: https://svelte-dev-git-preview-svelte-14816-svelte.vercel.app/

this is an automated message

@github-actions GitHub Actions
Copy link
Contributor

Playground

pnpm add https://pkg.pr.new/svelte@14816

paoloricciuti
paoloricciuti reviewed Dec 24, 2024
View reviewed changes
Copy link
Member
@paoloricciuti paoloricciuti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall is good...however we recently switched to let TS handle this kind of typing issues so maybe we can just prevent the fail without throwing the warning.

In this case it's a bit different tho because this will be a much more descriptive error... I'll let some other maintainer with more experience decide on this 😁

@GauBen
8000 Copy link
Contributor Author
GauBen commented Dec 24, 2024

Thanks for the review @paoloricciuti!

For consistency, I'd say that <select multiple value={null / undefined}> should be accepted and ignored, like how other properties behave when given a nullish value. Regarding non-array values, TS can do the trick, we can hard-crash on them.

@dummdidumm
Copy link
Member

What is the reason for allowing undefined/null? I cannot imagine a case where this would be useful.

Nor should we just silently bail, IMO. I instead propose to - only in dev mode - fail with a more specific error message if you did not provide an array.

@GauBen
Copy link
Contributor Author
GauBen commented Jan 7, 2025

What is the reason for allowing undefined/null?

I have a <Select> component that is just a <select> with additional styles: https://svelte.dev/playground/c4ab7f03ec304ab7b1cafb4fc254826c?version=5.16.6

My current workaround is

https://github.com/GauBen/timeline/blob/30534beb11f439fbee3089582f887df5be3db796/packages/uistiti/src/lib/Select.svelte#L14-L16

It's not very elegant, but I haven't found a better way

@svelte-docs-bot
Copy link

Preview: https://svelte-dev-git-preview-svelte-14816-svelte.vercel.app/

Rich-Harris
Rich-Harris approved these changes May 20, 2025
View reviewed changes
Copy link
Member
@Rich-Harris Rich-Harris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you! apologies for the slow merge

@Rich-Harris Rich-Harris merged commit de80949 into sveltejs:main May 20, 2025
9 checks passed
@github-actions github-actions bot mentioned this pull request May 20, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paoloricciuti paoloricciuti paoloricciuti left review comments

@Rich-Harris Rich-Harris Rich-Harris approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

<select multiple value={123}> crashes the runtime
4 participants
@GauBen @Rich-Harris @dummdidumm @paoloricciuti
0