fix: make defaultValue work with spread
#14640
Conversation
🦋 Changeset detectedLatest commit: 12780ed The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
|
Lovely! |
Do we just ignore that edge case and hope for the best? 😄 |
| } else if (key === 'defaultValue') { | ||
| /** @type {HTMLInputElement} */ (element).defaultValue = value; | ||
| } else if (key === 'defaultChecked') { | ||
| /** @type {HTMLInputElement} */ (element).defaultChecked = value; |
There was a problem hiding this comment.
I think we can remove those in favor of doing value == null && !is_custom_element && key !== 'defaultValue' && key === 'defaultChecked' below
There was a problem hiding this comment.
I think we can't because we also want to nullify it if the value is nullish right?
There was a problem hiding this comment.
Oh no wait it's because in the else if we are checking for setters.include
| assert.htmlEqual(test2_span.innerHTML, 'foo foo foo foo'); | ||
|
|
||
| after_reset.push(() => { | ||
| console.log('-------------'); |
There was a problem hiding this comment.
| console.log('-------------'); |
There was a problem hiding this comment.
This is also in your test for default value, should i delete those there too?
There was a problem hiding this comment.
oh woops, yeah do that then 😄
| assert.htmlEqual(test1_span.innerHTML, 'foo foo foo foo'); | ||
|
|
||
| after_reset.push(() => { | ||
| console.log('-------------'); |
There was a problem hiding this comment.
| console.log('-------------'); |
| @@ -368,7 +368,10 @@ export function set_attributes( | |||
| } else if (key === 'checked') { | |||
| /**@type {HTMLInputElement}*/ (element).defaultChecked = default_checked_reset; | |||
| } | |||
| } else if (setters.includes(name) && (is_custom_element || typeof value !== 'string')) { | |||
| } else if ( | |||
| is_default_value_or_checked || | |||
There was a problem hiding this comment.
do we need this check? AFAIK the setters should include defaultValue/defaultChecked
There was a problem hiding this comment.
Before this it was not working at all so I assumed no (tbh I didn't check)
There was a problem hiding this comment.
setters does include those values, but defaultValue fails the typeof value !== 'string' part of the check
| attributes[key] = null; | ||
| // if we remove the value/checked attributes this also for some reasons reset | ||
| // the default value so we need to keep track of it and reassign it after the remove |
There was a problem hiding this comment.
Ah I see, yeah that's why - now I'm wondering if we should model this differently: Check if the dom element is an input or textarea, and if so, if we come across a value or checked attribute, skip to the setters. Not sure which solution ends up being less code.
There was a problem hiding this comment.
I think we still need to remove the attribute, we just need to make sure to restore the default value
This is a WIP, which I'm not sure if it goes anywhere. It started out to deeper understand the fix in #14640, and to fix some more theoretical loopholes, but this turns out to not fix the issue yet, and I'm not sure if it even will, so I'm punting on it for now but putting it up for others to see.
|
preview: https://svelte-dev-git-preview-svelte-14640-svelte.vercel.app/ this is an automated message |
Rich-Harris
force-pushed
the
default-value-spread
branch
from
c4851b1 to
ce373ff
Compare
Closes #14634
I was not able to fully port the test without spreads because there's one single test I tried and failed to fix which is the textarea with
defaultValueand achild. Do anyone has any idea on how could i fix this?Before submitting the PR, please make sure you do the following
feat:,fix:,chore:, ordocs:.packages/svelte/src, add a changeset (npx changeset).Tests and linting
pnpm testand lint the project withpnpm lint