qwik icon indicating copy to clipboard operation
qwik copied to clipboard

[🐞] bind:value is broken when part of props spreading

Open cmbartschat opened this issue 2 years ago • 8 comments

Which component is affected?

Qwik Runtime

Describe the bug

If I define:

const Input = component$((props: any) => {
  return <input {...props} />;
});

That doesn't work to set bind:value={xxx} on Input.

But this implementation does work:

const FixedInput = component$(({ "bind:value": bindValue, ...rest }: any) => {
  return <input {...rest} bind:value={bindValue} />;
});

Reproduction

https://qwik.builder.io/playground/#version=0.103.0&buildMode=development&entryStrategy=hook&files=JTVCJTdCJTIycGF0aCUyMiUzQSUyMiUyRmFwcC50c3glMjIlMkMlMjJjb2RlJTIyJTNBJTIyaW1wb3J0JTIwJTdCJTIwY29tcG9uZW50JTI0JTJDJTIwdXNlU2lnbmFsJTIwJTdEJTIwZnJvbSUyMCU1QyUyMiU0MGJ1aWxkZXIuaW8lMkZxd2lrJTVDJTIyJTNCJTVDbiU1Q25jb25zdCUyMElucHV0JTIwJTNEJTIwY29tcG9uZW50JTI0KChwcm9wcyUzQSUyMGFueSklMjAlM0QlM0UlMjAlN0IlNUNuJTIwJTIwcmV0dXJuJTIwJTNDaW5wdXQlMjAlN0IuLi5wcm9wcyU3RCUyMCUyRiUzRSUzQiU1Q24lN0QpJTNCJTVDbiU1Q25jb25zdCUyMEZpeGVkSW5wdXQlMjAlM0QlMjBjb21wb25lbnQlMjQoKCU3QiUyMCU1QyUyMmJpbmQlM0F2YWx1ZSU1QyUyMiUzQSUyMGJpbmRWYWx1ZSUyQyUyMC4uLnJlc3QlMjAlN0QlM0ElMjBhbnkpJTIwJTNEJTNFJTIwJTdCJTVDbiUyMCUyMHJldHVybiUyMCUzQ2lucHV0JTIwJTdCLi4ucmVzdCU3RCUyMGJpbmQlM0F2YWx1ZSUzRCU3QmJpbmRWYWx1ZSU3RCUyMCUyRiUzRSUzQiU1Q24lN0QpJTNCJTVDbiU1Q25jb25zdCUyMExpdGVJbnB1dCUyMCUzRCUyMChwcm9wcyUzQSUyMGFueSklMjAlM0QlM0UlMjAlN0IlNUNuJTIwJTIwcmV0dXJuJTIwJTNDaW5wdXQlMjAlN0IuLi5wcm9wcyU3RCUyMCUyRiUzRSUzQiU1Q24lN0QlM0IlNUNuJTVDbmNvbnN0JTIwVGVzdDElMjAlM0QlMjBjb21wb25lbnQlMjQoKCklMjAlM0QlM0UlMjAlN0IlNUNuJTIwJTIwY29uc3QlMjB2YWx1ZSUyMCUzRCUyMHVzZVNpZ25hbCglNUMlMjJ0ZXN0MSU1QyUyMiklM0IlNUNuJTIwJTIwcmV0dXJuJTIwKCU1Q24lMjAlMjAlMjAlMjAlM0MlM0UlNUNuJTIwJTIwJTIwJTIwJTIwJTIwJTNDaDIlM0VUZXN0JTIwMSUzQyUyRmgyJTNFJTVDbiUyMCUyMCUyMCUyMCUyMCUyMCUzQ0lucHV0JTIwYmluZCUzQXZhbHVlJTNEJTdCdmFsdWUlN0QlMjAlMkYlM0UlNUNuJTIwJTIwJTIwJTIwJTIwJTIwJTNDcCUzRSU3QnZhbHVlJTdEJTNDJTJGcCUzRSU1Q24lMjAlMjAlMjAlMjAlM0MlMkYlM0UlNUNuJTIwJTIwKSUzQiU1Q24lN0QpJTNCJTVDbiU1Q25jb25zdCUyMFRlc3QyJTIwJTNEJTIwY29tcG9uZW50JTI0KCgpJTIwJTNEJTNFJTIwJTdCJTVDbiUyMCUyMGNvbnN0JTIwdmFsdWUlMjAlM0QlMjB1c2VTaWduYWwoJTVDJTIydGVzdDIlNUMlMjIpJTNCJTVDbiUyMCUyMHJldHVybiUyMCglNUNuJTIwJTIwJTIwJTIwJTNDJTNFJTVDbiUyMCUyMCUyMCUyMCUyMCUyMCUzQ2gyJTNFVGVzdCUyMDIlM0MlMkZoMiUzRSU1Q24lMjAlMjAlMjAlMjAlMjAlMjAlM0NMaXRlSW5wdXQlMjBiaW5kJTNBdmFsdWUlM0QlN0J2YWx1ZSU3RCUyMCUyRiUzRSU1Q24lMjAlMjAlMjAlMjAlMjAlMjAlM0NwJTNFJTdCdmFsdWUlN0QlM0MlMkZwJTNFJTVDbiUyMCUyMCUyMCUyMCUzQyUyRiUzRSU1Q24lMjAlMjApJTNCJTVDbiU3RCklM0IlNUNuJTVDbmNvbnN0JTIwVGVzdDMlMjAlM0QlMjBjb21wb25lbnQlMjQoKCklMjAlM0QlM0UlMjAlN0IlNUNuJTIwJTIwY29uc3QlMjB2YWx1ZSUyMCUzRCUyMHVzZVNpZ25hbCglNUMlMjJ0ZXN0MyU1QyUyMiklM0IlNUNuJTIwJTIwcmV0dXJuJTIwKCU1Q24lMjAlMjAlMjAlMjAlM0MlM0UlNUNuJTIwJTIwJTIwJTIwJTIwJTIwJTNDaDIlM0VUZXN0JTIwMyUzQyUyRmgyJTNFJTVDbiUyMCUyMCUyMCUyMCUyMCUyMCUzQ2lucHV0JTIwYmluZCUzQXZhbHVlJTNEJTdCdmFsdWUlN0QlMjAlMkYlM0UlNUNuJTIwJTIwJTIwJTIwJTIwJTIwJTNDcCUzRSU3QnZhbHVlJTdEJTNDJTJGcCUzRSU1Q24lMjAlMjAlMjAlMjAlM0MlMkYlM0UlNUNuJTIwJTIwKSUzQiU1Q24lN0QpJTNCJTVDbiU1Q25jb25zdCUyMFRlc3Q0JTIwJTNEJTIwY29tcG9uZW50JTI0KCgpJTIwJTNEJTNFJTIwJTdCJTVDbiUyMCUyMGNvbnN0JTIwdmFsdWUlMjAlM0QlMjB1c2VTaWduYWwoJTVDJTIydGVzdDQlNUMlMjIpJTNCJTVDbiUyMCUyMHJldHVybiUyMCglNUNuJTIwJTIwJTIwJTIwJTNDJTNFJTVDbiUyMCUyMCUyMCUyMCUyMCUyMCUzQ2gyJTNFVGVzdCUyMDQlM0MlMkZoMiUzRSU1Q24lMjAlMjAlMjAlMjAlMjAlMjAlM0NGaXhlZElucHV0JTIwYmluZCUzQXZhbHVlJTNEJTdCdmFsdWUlN0QlMjAlMkYlM0UlNUNuJTIwJTIwJTIwJTIwJTIwJTIwJTNDcCUzRSU3QnZhbHVlJTdEJTNDJTJGcCUzRSU1Q24lMjAlMjAlMjAlMjAlM0MlMkYlM0UlNUNuJTIwJTIwKSUzQiU1Q24lN0QpJTNCJTVDbiU1Q25leHBvcnQlMjBkZWZhdWx0JTIwY29tcG9uZW50JTI0KCgpJTIwJTNEJTNFJTIwJTdCJTVDbiUyMCUyMHJldHVybiUyMCglNUNuJTIwJTIwJTIwJTIwJTNDJTNFJTVDbiUyMCUyMCUyMCUyMCUyMCUyMCUzQ1Rlc3QxJTIwJTJGJTNFJTVDbiUyMCUyMCUyMCUyMCUyMCUyMCUzQ1Rlc3QyJTIwJTJGJTNFJTVDbiUyMCUyMCUyMCUyMCUyMCUyMCUzQ1Rlc3QzJTIwJTJGJTNFJTVDbiUyMCUyMCUyMCUyMCUyMCUyMCUzQ1Rlc3Q0JTIwJTJGJTNFJTVDbiUyMCUyMCUyMCUyMCUzQyUyRiUzRSU1Q24lMjAlMjApJTNCJTVDbiU3RCklM0IlNUNuJTIyJTdEJTJDJTdCJTIycGF0aCUyMiUzQSUyMiUyRmVudHJ5LnNlcnZlci50c3glMjIlMkMlMjJjb2RlJTIyJTNBJTIyaW1wb3J0JTIwJTdCJTIwcmVuZGVyVG9TdHJpbmclMkMlMjB0eXBlJTIwUmVuZGVyT3B0aW9ucyUyMCU3RCUyMGZyb20lMjAnJTQwYnVpbGRlci5pbyUyRnF3aWslMkZzZXJ2ZXInJTNCJTVDbmltcG9ydCUyMCU3QiUyMFJvb3QlMjAlN0QlMjBmcm9tJTIwJy4lMkZyb290JyUzQiU1Q24lNUNuZXhwb3J0JTIwZGVmYXVsdCUyMGZ1bmN0aW9uJTIwKG9wdHMlM0ElMjBSZW5kZXJPcHRpb25zKSUyMCU3QiU1Q24lMjAlMjByZXR1cm4lMjByZW5kZXJUb1N0cmluZyglM0NSb290JTIwJTJGJTNFJTJDJTIwb3B0cyklM0IlNUNuJTdEJTVDbiUyMiU3RCUyQyU3QiUyMnBhdGglMjIlM0ElMjIlMkZyb290LnRzeCUyMiUyQyUyMmNvZGUlMjIlM0ElMjJpbXBvcnQlMjBBcHAlMjBmcm9tJTIwJy4lMkZhcHAnJTNCJTVDbiU1Q25leHBvcnQlMjBjb25zdCUyMFJvb3QlMjAlM0QlMjAoKSUyMCUzRCUzRSUyMCU3QiU1Q24lMjAlMjByZXR1cm4lMjAoJTVDbiUyMCUyMCUyMCUyMCUzQyUzRSU1Q24lMjAlMjAlMjAlMjAlMjAlMjAlM0NoZWFkJTNFJTVDbiUyMCUyMCUyMCUyMCUyMCUyMCUyMCUyMCUzQ3RpdGxlJTNFSGVsbG8lMjBRd2lrJTNDJTJGdGl0bGUlM0UlNUNuJTIwJTIwJTIwJTIwJTIwJTIwJTNDJTJGaGVhZCUzRSU1Q24lMjAlMjAlMjAlMjAlMjAlMjAlM0Nib2R5JTNFJTVDbiUyMCUyMCUyMCUyMCUyMCUyMCUyMCUyMCUzQ0FwcCUyMCUyRiUzRSU1Q24lMjAlMjAlMjAlMjAlMjAlMjAlM0MlMkZib2R5JTNFJTVDbiUyMCUyMCUyMCUyMCUzQyUyRiUzRSU1Q24lMjAlMjApJTNCJTVDbiU3RCUzQiU1Q24lMjIlN0QlNUQ%3D

Steps to reproduce

  • Type some text under Test 1/Test 2 and see they don't work
  • Test 3 works, it uses a native <input>
  • Test 4 works, it has a manual property set for bind:value.

System Info

Qwik Playground/Windows/Firefox

Additional Information

No response

cmbartschat avatar Apr 26 '23 02:04 cmbartschat

So, today bind:value is a compiler thing, it does not really exist as a prop, in general prop spreading is a practice that removes all compiler optimizations. I cant think of a way to make this work. But we should document it and print some warning in production.

Not sure this helps, but i cant think of an easy fix right now!

manucorporat avatar Apr 26 '23 06:04 manucorporat

If the compilation step is a simple bind:value => value/onInput$ translation, does that only apply to a specific set of elements like input/select? If that translation applied to the custom <Input> as well it seems like we'd be fine.

Another concern here is if I use styled components to define a StyledInput, will Qwik recognize that it needs the bind:value translation as well? Or is it only compatible if I set it on a raw <input>?

cmbartschat avatar Apr 26 '23 08:04 cmbartschat

mmm, maybe for functional components we can leave the bind:value prop and add the value + onInput. ie, passing 3 props.

manucorporat avatar Apr 26 '23 08:04 manucorporat

@cmbartschat looks like we have to do something like that:

 {bind && (
        <input
          {...rest}
          type={type}
          placeholder={placeholder ?? ''}
          bind:value={bind}
        />
      )}
      {bind === undefined && (
        <input {...rest} type={type} placeholder={placeholder ?? ''} />
      )}

Very ugly, but there is no other way.

@manucorporat This will be a crucial part for all UI component libraries. @shairez how do you guys do that with QwikUI ?

tzdesign avatar Aug 18 '23 12:08 tzdesign

Good workaround! I've just been doing this 🤦

export const Input = component$(
  ({ class: className, "bind:value": bindValue, ...rest }: Props) => {
    return (
      <input
        type="text"
        class={[serializeClass(className), styles.input]}
        bind:value={bindValue}
        {...rest}
      />
    );
  }
);

export const InputNoBind = component$(
  ({ class: className, ...rest }: Props) => {
    return (
      <input
        type="text"
        class={[serializeClass(className), styles.input]}
        {...rest}
      />
    );
  }
);

cmbartschat avatar Aug 19 '23 04:08 cmbartschat

It's not really a good workaround. We will probably not use bind:value and omit it later. It's too confusing. The text field component is so complex (input or text area) that we will not duplicate the jsx for the sake of bind:value.

tzdesign avatar Aug 19 '23 07:08 tzdesign

Here's my workaround:

import {$, component$, type PropsOf} from '@builder.io/qwik'

export const TextField = component$(
	({
		label,
		class: c,
		['bind:value']: sig,
		...rest
	}: {label: string} & PropsOf<'input'> & {type?: 'text'}) => {
		// workaround for spread props not working with plain tags
		const Input = 'input'
		return (
			<label class="form-control w-full">
				<div class="label">
					<span class="label-text">{label}</span>
				</div>
				<Input
					value={sig?.value}
					onInput$={sig && $((__, el) => (sig.value = el.value))}
					{...rest}
					type="text"
					class={['input w-full', !rest.disabled && 'input-bordered', c]}
				/>
			</label>
		)
	}
)

@manucorporat why can't this be implemented in _jsxS?

wmertens avatar Jan 19 '24 15:01 wmertens

I took a look at this, the problem is adding a QRL from _jsxS, I can't make that work. If that works, the special handling can go out of the optimizer and into _jsxS, which makes it work even when passing via spread props.

wmertens avatar Mar 12 '24 15:03 wmertens

@wmertens I tried your workaround and it works for bind: but breaks the normal usage of value/onInput$

Here's my workaround:


type InputProps = PropsOf<'input'>;

export const Input = component$<InputProps>(
  ({ ['bind:value']: valueSig, value, onInput$, ...props }) => {
    return (
      <>
        <input
          {...props}
          value={valueSig ? valueSig.value : value}
          onInput$={valueSig ? $((__, el) => (valueSig.value = el.value)) : onInput$}
        />
      </>
    );
  },
);

maiieul avatar May 26 '24 14:05 maiieul