solid icon indicating copy to clipboard operation
solid copied to clipboard

<input value={undefined}> shows string "undefined"

Open edemaine opened this issue 3 years ago • 5 comments

Describe the bug

This is a sequel to #955, where the same issue occurred with class/className.

<input value={undefined}/>

compiles to

const _el$ = _tmpl$.cloneNode(true);
_el$.value = undefined;
return _el$;

Because of the infinite wisdom of the DOM .value interface, this causes the value to be set to the string "undefined". 🙁

Reported on Discord

Your Example Website or App

https://playground.solidjs.com/?hash=-335339517&version=1.4.1

Steps to Reproduce the Bug or Issue

  1. Go to Playground link
  2. Observe input value.

Expected behavior

I'm not sure there's a clear expectation here, but I probably expected a blank input, with the intuition that attr={undefined} is equivalent to omitting attr altogether. Instead I'm seeing the string "undefined" in the input.

Screenshots or Videos

image

Platform

  • OS: Windows
  • Browser: Chrome
  • Version: 102

Additional context

FWIW, here's a CodeSandbox showing that value={undefined} renders blank in React.

Is it worth reviewing for other DOM interfaces like these?

edemaine avatar Jun 06 '22 00:06 edemaine

It's only going to be the short list of properties I choose to handle as properties for performance. I just double-checked and it's value and className and boolean attributes. Unfortunately adding checks both increases bundles size and worsens performance. Although not as much as using attributes I suppose.

I think if there is a good reason for a value helper probably should consider it. What finally pushed className was the desire to actually remove the attribute. I think people come across that one more because they write conditional logic. With value you probably only hit this if you are initializing and not using TypeScript.

The solution is to initialize to an empty string. It is so much better if we can just not do anything here as it is so easy to mitigate and the solution puts a size and perf tax on everyone. But if there was more reason to go this way definitely do it.

ryansolid avatar Jun 06 '22 02:06 ryansolid

Agreed; I expect a type error when setting value to undefined, so it seems fine to leave this behavior, provided we document and perhaps help avoid it.

  1. In TypeScript, I don't actually see a type error here. I guess this requires exactOptionalPropertyTypes which was brought up recently. If we can properly distinguish between "optional" (?) and "undefined allowed" (| undefined) we could recommend people turn on this option.

  2. In dev mode, we could detect this and issue a warning. Though it's pretty obvious what's going on here when you see it, so I'm not sure this is necessary.

edemaine avatar Jun 06 '22 03:06 edemaine

I just encountered this after updating an older project. Curious when this behaviour changed as it didn't seem to happen before?

martinpengellyphillips avatar Sep 19 '22 17:09 martinpengellyphillips

Always been like this. This is an example where we'd need to do more work to prevent it.

ryansolid avatar Sep 20 '22 08:09 ryansolid